Conversation
src/reader.rs
Outdated
| // Redraw the command line. This is what ensures the autosuggestion is hidden, | ||
| // transient prompt is drawn, etc. after the user presses enter. | ||
| if zelf.exec_transient_prompt() | ||
| || zelf.is_repaint_needed(None) |
There was a problem hiding this comment.
I'm not yet sure what's the best solution but I'll leave a brain dump here.
fish_transient_prompt and fish_transient_right_prompt both do two things:
- tell fish that we want to repaint before exec
- print the final prompt contents
I wonder if we can get rid of the exponential explosion (4 combinations with the choices about transience and left/right).
We'd need a different way to communicate 1 and 2.
Another issue is that fish_transient_prompt is a weird name because that prompt is the one
that will stick around -- fish_prompt will be the transient one.
Maybe instead of adding new prompt names, we should
- add a boolean
fish_transient_modevariable. If true, we always repaint before exec - pass a flag to the prompt like
fish_prompt --pending-execso prompts can alter their appearance based oncontains --pending-exec $argv
Should try to find a better name, since it also applies toctrl-d
Looks like existing transient prompts (tide and transient.fish)
override enter and ctrl-d bindings, so I guess this native approach is nicer.
To the question of whether preexec can do this
(can ignore this since the answer seems to be "doesn't seem feasible").
Historically, all readline commands went through an input queue, even if they were triggered by
commandline -f.
This changed in a583fe7 ("commandline -f foo" to skip queue and execute immediately,
2024-04-08).
Repaints commands are the exception, they still use the keyboard input queue
(5ba21cd (Send repaint requests through the input queue again, 2024-04-19)),
the immediate reason was $status.
Commit a19ff49 (Prevent out-of-order execution following repaint, 2024-05-13) suggests that
we could maybe get rid of the exception and lie about $status, $pipestatus etc.
However commit cfcf415 (Render overflown commandline in entirety just before executing,
2024-11-02) made the last repaint special. This means that a synchronous redraw in --on-event fish_preexec wouldn't really. fish wants to know which repaint is the last one and if there
is one after layout_and_repaint_before_execution(), that seems too hard.
There was a problem hiding this comment.
Looks like existing transient prompts (tide and transient.fish)
override enter and ctrl-d bindings, so I guess this native approach is nicer.
Don't 100% follow the rest of this comment but this is exactly why I originally raised #11101, rebinding default binds to custom functions is clearly a suboptimal approach (also discussed a bit in #7602 (comment)).
Mainly due to the custom function now being left to handle all edge cases that result from wrappping the default bind along with requiring rebinds of any key that would cause a prompt redraw, i.e. what is happening in transient.fish: https://github.com/zzhaolei/transient.fish/blob/7091a1ef574e4c2d16779e59d37ceb567128c787/conf.d/transient.fish#L38-L58 whereas tide doesnt even attempt this.
Any solution that avoids default rebinds is good enough for me, and would greatly simplify lots of potentially duplicated logic that would come about from writing a "complete" transient prompt implementation.
That being said, putting aside the valid naming issues and any potential performance implications, the approach that this PR takes is probably going to be the simplest for the end user to understand compared to any combination of booleans/flags.
There was a problem hiding this comment.
Introducing fish_transient_mode eliminates the need for excess functions, but it also complicates things if you want only one of the prompts to be reexecuted (the best solution I can think of is to save the prompt to an environment variable and print its content upon receiving the flag). But I don’t have a particular preference between my current solution and using this approach.
Also, should cancel-commandline update the prompt?
There was a problem hiding this comment.
fish_transient_mode complicates things if you want only one of the prompts to be reexecuted
We could re-read this global variable after every time we execute the prompt, capturing the current value for each prompt.
Also, should cancel-commandline update the prompt?
not sure but I guess no. I wonder what are the prompts that want redrawing (maybe date or git status).
There was a problem hiding this comment.
I think it's fine to merge as-is but if anyone wants more flexibility, we should consider that change.
Essentially the global fish_transient_prompt will be an output variable of the fish_prompt family of functions.
I think it makes sense to run set -g fish_transient_prompt 1 inside fish_prompt,
because that allows each prompt to specify exactly when it supports transient prompt (though I guess setting it to 1 always is also harmless).
Of course that would mean that plugins can't switch on fish_transient_prompt but I'm not sure if they should - they can simply define their own variable that is intended to be toggled by the user etc.
I will point out that the other alternative to fish_transient_prompt is function fish_prompt --argument-names is_final_rendering.
But these are positional arguments; I don't think we want to guarantee an order here
3726805 to
650b0b0
Compare
|
Now, before final rendering, fish will check if a function is marked as transient (using function fish_prompt --transient
if contains -- --final-rendering $argv
echo 'final> '
else
echo 'transient> '
end
end |
An entire option to I'd probably do what @krobelus said and add a $fish_transient_prompt variable (and I would make it "prompt", not "mode" because that connects it to the prompt) that can be set to "1" or "0". If there is demand to have only the left prompt redraw it could also use a value of "left" or "right", but tbh I don't see that being a thing - as far as I can tell transient prompts are really only used to collapse them before execution, which would apply to both. |
| :class: highlight | ||
|
|
||
| :green:`~/M/L/Oneknowing`>false | ||
| :green:`~/M/L/Oneknowing`\ :red:`[1]`>_ |
There was a problem hiding this comment.
This is a nice example. I think we should even consider making this the default.
Our default prompt is a bit noisy given long pipelines that fail (due to the last command exiting with nonzero)
user@localhost ~/g/fish-shell > echo foo | cat | cat | grep not-found
user@localhost ~/g/fish-shell [0|0|0|1]>
The exit status is probaly not super interesting to have stick around?
(Going further, I think collapsing the final rendering to a mere $ would be a good idea for lots of use cases but that's another discussion)
src/reader.rs
Outdated
|
|
||
| fn check_autosuggestion_enabled(vars: &dyn Environment) -> bool { | ||
| vars.get(L!("fish_autosuggestion_enabled")) | ||
| fn check_var(vars: &dyn Environment, name: &wstr, default: bool) -> bool { |
There was a problem hiding this comment.
I'd probably call this check_bool_var but I guess it doesn't matter, it's obvious enough.
(it always trips me up that I can't use set fish_autosuggestion_enabled false but that's a different issue).
src/reader.rs
Outdated
| ) | ||
| < 8000 span class="blob-code-inner blob-code-marker-context"> }, | ||
| true, | ||
| (true, false), |
There was a problem hiding this comment.
These scope guards are unwieldy because they force the user to spell
out std::mem::replace etc.
Defining two different things in a scope guard seems like a workaround for the high amount of boilerplate needed.
I think we should instead define a separate scope guard (let mut zelf = scoped_push_replacer_ctx(zelf, ...)).
In future we can get rid of the boilerplate. I think it's worth adding
a macro. I'd probably go with this, feel free to insert this commit
first if it feels ready.
Details
From: Johannes Altmanninger <aclopte@gmail.com>
Date: Sat, 22 Feb 2025 09:45:08 +0100
Subject: [PATCH] Rework scoped-replace ergonomics
We have several helpers functions that set a value and return a
ScopeGuard that restores the old value.
The original helper takes a function argument that returns a mutable
reference to the value in question. But when we're using interior
mutability (Cell's RefMut, atomics), we cannot (easily) get a mutable
reference. As a result, the most commonly used helper is the awkwardly
named "scoped_push_replacer()", which takes a function argument with
the "std::mem::replace()" signature.
The most common use is to set some value in "Parser::libdata_mut()".
This requires a lot of boilerplate for setting a single value.
Let's introduce a macro to express this behavior more directly.
While at it,
1. Rename "scoped_push_replacer" to "scoped_replace" etc., I don't
think "push" is necessary now that we have the Rusty "replace".
2. Since the original helper is only used once (in non-test code),
so get rid of it in
10000
favor of the more general version.
3. Add another macro for the cases where surrounding code requires
exclusive access to the Reader.
---
src/bin/fish.rs | 21 +++++++-------
src/builtins/read.rs | 7 +----
src/builtins/source.rs | 8 ++---
src/common.rs | 22 ++------------
src/event.rs | 16 ++++------
src/exec.rs | 14 ++++-----
src/lib.rs | 2 ++
src/parse_execution.rs | 13 ++++-----
src/parser.rs | 7 ++---
src/parser_macros.rs | 9 ++++++
src/proc.rs | 9 ++----
src/reader.rs | 66 ++++++++++++++----------------------------
src/tests/common.rs | 29 +++++++++++++++----
src/wildcard.rs | 8 +++--
14 files changed, 102 insertions(+), 129 deletions(-)
create mode 100644 src/parser_macros.rs
diff --git a/src/bin/fish.rs b/src/bin/fish.rs
index 7ab4cc3d61..0d11d0ba53 100644
--- a/src/bin/fish.rs
+++ b/src/bin/fish.rs
@@ -27,14 +27,13 @@
use fish::future::IsSomeAnd;
use fish::{
ast::Ast,
- builtins::fish_indent,
- builtins::fish_key_reader,
- builtins::shared::{
- BUILTIN_ERR_MISSING, BUILTIN_ERR_UNKNOWN, STATUS_CMD_OK, STATUS_CMD_UNKNOWN,
+ builtins::{
+ fish_indent, fish_key_reader,
+ shared::{BUILTIN_ERR_MISSING, BUILTIN_ERR_UNKNOWN, STATUS_CMD_OK, STATUS_CMD_UNKNOWN},
},
common::{
- escape, get_executable_path, save_term_foreground_process_group, scoped_push_replacer,
- str2wcstring, wcs2string, PACKAGE_NAME, PROFILING_ACTIVE, PROGRAM_NAME,
+ escape, get_executable_path, save_term_foreground_process_group, str2wcstring, wcs2string,
+ PACKAGE_NAME, PROFILING_ACTIVE, PROGRAM_NAME,
},
env::{
environment::{env_init, EnvStack, Environment},
@@ -59,6 +58,7 @@
set_interactive_session, Pid,
},
reader::{reader_init, reader_read, term_copy_modes},
+ scoped_replace_libdata,
signal::{signal_clear_cancel, signal_unblock_all},
threads::{self},
topic_monitor,
@@ -947,11 +947,10 @@ fn throwing_main() -> i32 {
list.iter().map(|s| s.to_owned()).collect(),
);
let rel_filename = &args[my_optind - 1];
- let _filename_push = scoped_push_replacer(
- |new_value| {
- std::mem::replace(&mut parser.libdata_mut().current_filename, new_value)
- },
- Some(Arc::new(rel_filename.to_owned())),
+ let _filename_push = scoped_replace_libdata!(
+ parser,
+ current_filename,
+ Some(Arc::new(rel_filename.to_owned()))
);
res = reader_read(parser, f.as_raw_fd(), &IoChain::new());
if res != 0 {
diff --git a/src/builtins/read.rs b/src/builtins/read.rs
index b25951af5e..339e8334f3 100644
--- a/src/builtins/read.rs
+++ b/src/builtins/read.rs
@@ -3,7 +3,6 @@
use super::prelude::*;
use crate::common::escape;
use crate::common::read_blocked;
-use crate::common::scoped_push_replacer;
use crate::common::str2wcstring;
use crate::common::unescape_string;
use crate::common::valid_var_name;
@@ -238,11 +237,7 @@ fn read_interactive(
commandline_set_buffer(Some(commandline.to_owned()), None);
let mline = {
- let _interactive = scoped_push_replacer(
- |new_value| std::mem::replace(&mut parser.libdata_mut().is_interactive, new_value),
- true,
- );
-
+ let _interactive = scoped_replace_libdata!(parser, is_interactive, true);
reader_readline(parser, nchars)
};
terminal_protocols_disable_ifn();
diff --git a/src/builtins/source.rs b/src/builtins/source.rs
index 6339d94f06..1758aa8069 100644
--- a/src/builtins/source.rs
+++ b/src/builtins/source.rs
@@ -1,7 +1,7 @@
use std::os::fd::AsRawFd;
use crate::{
- common::{escape, scoped_push_replacer, FilenameRef},
+ common::{escape, FilenameRef},
fds::wopen_cloexec,
nix::isatty,
parser::Block,
@@ -78,10 +78,8 @@ pub fn source(parser: &Parser, streams: &mut IoStreams, args: &mut [&wstr]) -> O
assert!(fd >= 0, "Should have a valid fd");
let sb = parser.push_block(Block::source_block(func_filename.clone()));
- let _filename_push = scoped_push_replacer(
- |new_value| std::mem::replace(&mut parser.libdata_mut().current_filename, new_value),
- Some(func_filename.clone()),
- );
+ let _filename_push =
+ scoped_replace_libdata!(parser, current_filename, Some(func_filename.clone()));
// Construct argv for the sourced file from our remaining args.
// This is slightly subtle. If this is a bare `source` with no args then `argv + optind` already
diff --git a/src/common.rs b/src/common.rs
index d4a8a32dae..6967eb7963 100644
--- a/src/common.rs
+++ b/src/common.rs
@@ -1763,24 +1763,7 @@ fn commit(guard: Self) -> T {
/// A scoped manager to save the current value of some variable, and set it to a new value. When
/// dropped, it restores the variable to its old value.
-pub fn scoped_push<Context, Accessor, T>(
- mut ctx: Context,
- accessor: Accessor,
- new_value: T,
-) -> impl ScopeGuarding<Target = Context>
-where
- Accessor: Fn(&mut Context) -> &mut T,
-{
- let saved = mem::replace(accessor(&mut ctx), new_value);
- let restore_saved = move |ctx: &mut Context| {
- *accessor(ctx) = saved;
- };
- ScopeGuard::new(ctx, restore_saved)
-}
-
-/// Similar to scoped_push but takes a function like "std::mem::replace" instead of a function
-/// that returns a mutable reference.
-pub fn scoped_push_replacer<Replacer, T>(
+pub fn scoped_replace<Replacer, T>(
replacer: Replacer,
new_value: T,
) -> impl ScopeGuarding<Target = ()>
@@ -1794,7 +1777,8 @@ pub fn scoped_push_replacer<Replacer, T>(
ScopeGuard::new((), restore_saved)
}
-pub fn scoped_push_replacer_ctx<Context, Replacer, T>(
+/// Like scoped_replace but also behave like the given context object.
+pub fn scoped_replace_with_context<Context, Replacer, T>(
mut ctx: Context,
replacer: Replacer,
new_value: T,
diff --git a/src/event.rs b/src/event.rs
index 9d40e47f24..20f8d607f1 100644
--- a/src/event.rs
+++ b/src/event.rs
@@ -7,7 +7,7 @@
use std::sync::atomic::{AtomicBool, AtomicU32, Ordering};
use std::sync::{Arc, Mutex};
-use crate::common::{escape, scoped_push_replacer, ScopeGuard};
+use crate::common::{escape, ScopeGuard};
use crate::flog::FLOG;
use crate::io::{IoChain, IoStreams};
use crate::job_group::MaybeJobId;
@@ -466,15 +466,11 @@ fn fire_internal(parser: &Parser, event: &Event) {
);
// Suppress fish_trace during events.
- let is_event = parser.libdata().is_event;
- let _inc_event = scoped_push_replacer(
- |new_value| std::mem::replace(&mut parser.libdata_mut().is_event, new_value),
- is_event + 1,
- );
- let _suppress_trace = scoped_push_replacer(
- |new_value| std::mem::replace(&mut parser.libdata_mut().suppress_fish_trace, new_value),
- true,
- );
+ let _inc_event = {
+ let is_event = parser.libdata().is_event;
+ scoped_replace_libdata!(parser, is_event, is_event + 1)
+ };
+ let _suppress_trace = scoped_replace_libdata!(parser, suppress_fish_trace, true);
// Capture the event handlers that match this event.
let fire: Vec<_> = EVENT_HANDLERS
diff --git a/src/exec.rs b/src/exec.rs
index bebd7dc8c0..fa83d43629 100644
--- a/src/exec.rs
+++ b/src/exec.rs
@@ -8,8 +8,8 @@
STATUS_READ_TOO_MUCH,
};
use crate::common::{
- exit_without_destructors, scoped_push_replacer, str2wcstring, truncate_at_nul, wcs2string,
- wcs2zstring, write_loop, ScopeGuard,
+ exit_without_destructors, str2wcstring, truncate_at_nul, wcs2string, wcs2zstring, write_loop,
+ ScopeGuard,
};
use crate::env::{EnvMode, EnvStack, Environment, Statuses, READ_BYTE_LIMIT};
use crate::env_dispatch::use_posix_spawn;
@@ -1450,12 +1450,10 @@ fn exec_subshell_internal(
is_subcmd: bool,
) -> libc::c_int {
parser.assert_can_execute();
- let _is_subshell = scoped_push_replacer(
- |new_value| std::mem::replace(&mut parser.libdata_mut().is_subshell, new_value),
- true,
- );
- let _read_limit = scoped_push_replacer(
- |new_value| std::mem::replace(&mut parser.libdata_mut().read_limit, new_value),
+ let _is_subshell = scoped_replace_libdata!(parser, is_subshell, true);
+ let _read_limit = scoped_replace_libdata!(
+ parser,
+ read_limit,
if is_subcmd {
READ_BYTE_LIMIT.load(Ordering::Relaxed)
} else {
diff --git a/src/lib.rs b/src/lib.rs
index 09252c5755..3e8e76e4b9 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -24,6 +24,8 @@
#[macro_use]
pub mod common;
+#[macro_use]
+pub mod parser_macros;
#[macro_use]
pub mod wutil;
diff --git a/src/parse_execution.rs b/src/parse_execution.rs
index d588d92bf4..f09ffd7bd6 100644
--- a/src/parse_execution.rs
+++ b/src/parse_execution.rs
@@ -11,8 +11,8 @@
STATUS_UNMATCHED_WILDCARD,
};
use crate::common::{
- escape, scoped_push_replacer, should_suppress_stderr_for_tests, truncate_at_nul,
- valid_var_name, ScopeGuard, ScopeGuarding,
+ escape, scoped_replace, should_suppress_stderr_for_tests, truncate_at_nul, valid_var_name,
+ ScopeGuard, ScopeGuarding,
};
use crate::complete::CompletionList;
use crate::env::{EnvMode, EnvStackSetResult, EnvVar, EnvVarFlags, Environment, Statuses};
@@ -1535,14 +1535,14 @@ fn run_1_job(
}
// Increment the eval_level for the duration of this command.
- let _saved_eval_level = scoped_push_replacer(
+ let _saved_eval_level = scoped_replace(
|new_value| ctx.parser().eval_level.swap(new_value, Ordering::Relaxed),
ctx.parser().eval_level.load(Ordering::Relaxed) + 1,
);
// Save the executing node.
let line_counter = Rc::clone(&self.line_counter);
- let _saved_node = scoped_push_replacer(
+ let _saved_node = scoped_replace(
|node| line_counter.borrow_mut().set_node(node),
Some(job_node),
);
@@ -1640,10 +1640,7 @@ fn run_1_job(
// We are about to populate a job. One possible argument to the job is a command substitution
// which may be interested in the job that's populating it, via '--on-job-exit caller'. Record
// the job ID here.
- let _caller_id = scoped_push_replacer(
- |new_value| std::mem::replace(&mut ctx.parser().libdata_mut().caller_id, new_value),
- job.internal_job_id,
- );
+ let _caller_id = scoped_replace_libdata!(ctx.parser(), caller_id, job.internal_job_id);
// Populate the job. This may fail for reasons like command_not_found. If this fails, an error
// will have been printed.
diff --git a/src/parser.rs b/src/parser.rs
index e6a2b4a83e..dc5667ed38 100644
--- a/src/parser.rs
+++ b/src/parser.rs
@@ -3,8 +3,8 @@
use crate::ast::{self, Ast, List, Node};
use crate::builtins::shared::STATUS_ILLEGAL_CMD;
use crate::common::{
- escape_string, scoped_push_replacer, CancelChecker, EscapeFlags, EscapeStringStyle,
- FilenameRef, ScopeGuarding, PROFILING_ACTIVE,
+ escape_string, scoped_replace, CancelChecker, EscapeFlags, EscapeStringStyle, FilenameRef,
+ ScopeGuarding, PROFILING_ACTIVE,
};
use crate::complete::CompletionList;
use crate::env::{EnvMode, EnvStack, EnvStackSetResult, Environment, Statuses};
@@ -603,8 +603,7 @@ pub fn eval_node<T: Node>(
// Restore the line counter.
let line_counter = Rc::clone(&self.line_counter);
- let scoped_line_counter =
- scoped_push_replacer(|v| line_counter.replace(v), ps.line_counter());
+ let scoped_line_counter = scoped_replace(|v| line_counter.replace(v), ps.line_counter());
// Create a new execution context.
let mut execution_context =
diff --git a/src/parser_macros.rs b/src/parser_macros.rs
new file mode 100644
index 0000000000..477f8f8fad
--- /dev/null
+++ b/src/parser_macros.rs
@@ -0,0 +1,9 @@
+#[macro_export]
+macro_rules! scoped_replace_libdata {
+ ( $parser:expr, $name:ident, $new_value:expr $(,)? ) => {
+ $crate::common::scoped_replace(
+ |new_value| std::mem::replace(&mut $parser.libdata_mut().$name, new_value),
+ $new_value,
+ )
+ };
+}
diff --git a/src/proc.rs b/src/proc.rs
index e2911d340a..82e491036f 100644
--- a/src/proc.rs
+++ b/src/proc.rs
@@ -4,8 +4,8 @@
use crate::ast;
use crate::common::{
- charptr2wcstring, escape, is_windows_subsystem_for_linux, redirect_tty_output,
- scoped_push_replacer, timef, Timepoint, WSL,
+ charptr2wcstring, escape, is_windows_subsystem_for_linux, redirect_tty_output, timef,
+ Timepoint, WSL,
};
use crate::env::Statuses;
use crate::event::{self, Event};
@@ -1783,10 +1783,7 @@ fn process_clean_after_marking(parser: &Parser, allow_interactive: bool) -> bool
return false;
}
- let _cleaning = scoped_push_replacer(
- |new_value| std::mem::replace(&mut parser.libdata_mut().is_cleaning_procs, new_value),
- true,
- );
+ let _cleaning = scoped_replace_libdata!(parser, is_cleaning_procs, true);
// This may be invoked in an exit handler, after the TERM has been torn down
// Don't try to print in that case (#3222)
diff --git a/src/reader.rs b/src/reader.rs
index 41f95c5b52..6309835676 100644
--- a/src/reader.rs
+++ b/src/reader.rs
@@ -50,9 +50,8 @@
use crate::common::restore_term_foreground_process_group_for_exit;
use crate::common::{
escape, escape_string, exit_without_destructors, get_ellipsis_char, get_obfuscation_read_char,
- redirect_tty_output, scoped_push_replacer, scoped_push_replacer_ctx, shell_modes, str2wcstring,
- wcs2string, write_loop, EscapeFlags, EscapeStringStyle, ScopeGuard, PROGRAM_NAME,
- UTF8_BOM_WCHAR,
+ redirect_tty_output, shell_modes, str2wcstring, wcs2string, write_loop, EscapeFlags,
+ EscapeStringStyle, ScopeGuard, PROGRAM_NAME, UTF8_BOM_WCHAR,
};
use crate::complete::{
complete, complete_load, sort_and_prioritize, CompleteFlags, Completion, CompletionList,
@@ -631,10 +630,7 @@ pub fn reader_read(parser: &Parser, fd: RawFd, io: &IoChain) -> c_int {
}
}
- let _interactive_push = scoped_push_replacer(
- |new_value| std::mem::replace(&mut parser.libdata_mut().is_interactive, new_value),
- interactive,
- );
+ let _interactive_push = scoped_replace_libdata!(parser, is_interactive, interactive);
signal_set_handlers_once(interactive);
let res = if interactive {
@@ -2138,6 +2134,18 @@ fn jump(
const QUERY_PRIMARY_DEVICE_ATTRIBUTE: &[u8] = b"\x1b[0c";
+macro_rules! scoped_replace_with_reader {
+ ( $reader:expr, $name:ident, $new_value:expr $(,)? ) => {{
+ crate::common::scoped_replace_with_context(
+ $reader,
+ |reader, new_value| {
+ std::mem::replace(&mut reader.parser.libdata_mut().$name, new_value)
+ },
+ $new_value,
+ )
+ }};
+}
+
impl<'a> Reader<'a> {
/// Read a command to execute, respecting input bindings.
/// Return the command, or none if we were asked to cancel (e.g. SIGHUP).
@@ -2146,16 +2154,7 @@ fn readline(&mut self, nchars: Option<NonZeroUsize>) -> Option<WString> {
// Suppress fish_trace during executing key bindings.
// This is simply to reduce noise.
- let mut zelf = scoped_push_replacer_ctx(
- self,
- |zelf, new_value| {
- std::mem::replace(
- &mut zelf.parser.libdata_mut().suppress_fish_trace,
- new_value,
- )
- },
- true,
- );
+ let mut zelf = scoped_replace_with_reader!(self, suppress_fish_trace, true);
// If nchars_or_0 is positive, then that's the maximum number of chars. Otherwise keep it at
// SIZE_MAX.
@@ -4488,14 +4487,8 @@ pub fn reader_write_title(
parser: &Parser,
reset_cursor_position: bool, /* = true */
) {
- let _noninteractive = scoped_push_replacer(
- |new_value| std::mem::replace(&mut parser.libdata_mut().is_interactive, new_value),
- false,
- );
- let _in_title = scoped_push_replacer(
- |new_value| std::mem::replace(&mut parser.libdata_mut().suppress_fish_trace, new_value),
- true,
- );
+ let _noninteractive = scoped_replace_libdata!(parser, is_interactive, false);
+ let _in_title = scoped_replace_libdata!(parser, suppress_fish_trace, true);
let mut fish_title_command = DEFAULT_TITLE.to_owned();
if function::exists(L!("fish_title"), parser) {
@@ -4540,17 +4533,8 @@ impl<'a> Reader<'a> {
fn exec_prompt(&mut self, exec_left: bool, exec_right: bool) {
// Suppress fish_trace while in the prompt.
// Prompts must be run non-interactively.
- let mut zelf = scoped_push_replacer_ctx(
- self,
- |zelf, new_value| {
- let mut libdata = zelf.parser.libdata_mut();
- (
- std::mem::replace(&mut libdata.suppress_fish_trace, new_value.0),
- std::mem::replace(&mut libdata.is_interactive, new_value.1),
- )
- },
- (true, false),
- );
+ let zelf = scoped_replace_with_reader!(self, suppress_fish_trace, true);
+ let mut zelf = scoped_replace_with_reader!(zelf, is_interactive, false);
// Update the termsize now.
// This allows prompts to react to $COLUMNS.
@@ -5348,10 +5332,7 @@ fn expand_replacer(
let mut cmd = escape(&repl.replacement);
cmd.push(' ');
cmd.push_utfstr(&escape(token));
- let _not_interactive = scoped_push_replacer(
- |new_value| std::mem::replace(&mut parser.libdata_mut().is_interactive, new_value),
- false,
- );
+ let _not_interactive = scoped_replace_libdata!(parser, is_interactive, false);
let mut outputs = vec![];
let ret = exec_subshell(
@@ -5796,10 +5777,7 @@ fn should_add_to_history(&mut self, text: &wstr) -> bool {
let mut cmd: WString = L!("fish_should_add_to_history ").into();
cmd.push_utfstr(&escape(text));
- let _not_interactive = scoped_push_replacer(
- |new_value| std::mem::replace(&mut parser.libdata_mut().is_interactive, new_value),
- false,
- );
+ let _not_interactive = scoped_replace_libdata!(parser, is_interactive, false);
let ret = exec_subshell(&cmd, parser, None, /*apply_exit_status=*/ false);
diff --git a/src/tests/common.rs b/src/tests/common.rs
index e1330d3ea4..b98bf18810 100644
--- a/src/tests/common.rs
+++ b/src/tests/common.rs
@@ -1,8 +1,9 @@
-use crate::common::{scoped_push, truncate_at_nul, ScopeGuard, ScopeGuarding};
+use crate::common::{scoped_replace_with_context, truncate_at_nul, ScopeGuard, ScopeGuarding};
use crate::wchar::prelude::*;
+use std::mem;
#[test]
-fn test_scoped_push() {
+fn test_scoped_replace() {
struct Context {
value: i32,
}
@@ -10,11 +11,19 @@ struct Context {
let mut value = 0;
let mut ctx = Context { value };
{
- let mut ctx = scoped_push(&mut ctx, |ctx| &mut ctx.value, value + 1);
+ let mut ctx = scoped_replace_with_context(
+ &mut ctx,
+ |ctx, new_value| mem::replace(&mut ctx.value, new_value),
+ value + 1,
+ );
value = ctx.value;
assert_eq!(value, 1);
{
- let mut ctx = scoped_push(&mut ctx, |ctx| &mut ctx.value, value + 1);
+ let mut ctx = scoped_replace_with_context(
+ &mut ctx,
+ |ctx, new_value| mem::replace(&mut ctx.value, new_value),
+ value + 1,
+ );
assert_eq!(ctx.value, 2);
ctx.value = 5;
assert_eq!(ctx.value, 5);
@@ -58,9 +67,17 @@ struct Storage {
}
let obj = Storage { value: "nu" };
assert_eq!(obj.value, "nu");
- let obj = scoped_push(obj, |obj| &mut obj.value, "mu");
+ let obj = scoped_replace_with_context(
+ obj,
+ |obj, new_value| mem::replace(&mut obj.value, new_value),
+ "mu",
+ );
assert_eq!(obj.value, "mu");
- let obj = scoped_push(obj, |obj| &mut obj.value, "mu2");
+ let obj = scoped_replace_with_context(
+ obj,
+ |obj, new_value| mem::replace(&mut obj.value, new_value),
+ "mu2",
+ );
assert_eq!(obj.value, "mu2");
let obj = ScopeGuarding::commit(obj);
assert_eq!(obj.value, "mu");
diff --git a/src/wildcard.rs b/src/wildcard.rs
index a84754ac0e..f98e13f0f2 100644
--- a/src/wildcard.rs
+++ b/src/wildcard.rs
@@ -437,7 +437,7 @@ mod expander {
use libc::F_OK;
use crate::{
- common::scoped_push,
+ common::scoped_replace_with_context,
path::append_path_component,
wutil::{dir_iter::DirIter, normalize_path, DevInode},
};
@@ -757,7 +757,11 @@ fn expand_literal_intermediate_segment_with_fuzz(
prefix: &wstr,
) {
// Mark that we are fuzzy for the duration of this function
- let mut zelf = scoped_push(self, |e| &mut e.has_fuzzy_ancestor, true);
+ let mut zelf = scoped_replace_with_context(
+ self,
+ |zelf, new_value| std::mem::replace(&mut zelf.has_fuzzy_ancestor, new_value),
+ true,
+ );
while !zelf.interrupted_or_overflowed() {
let Some(Ok(entry)) = base_dir_iter.next() else {
break;
--
2.48.1
src/reader.rs
Outdated
| false, | ||
| ); | ||
| // We do not support multiline mode indicators, so just concatenate all of them. | ||
| zelf.mode_prompt_buff = WString::from_iter(mode_indicator_list); |
There was a problem hiding this comment.
Right, this is how things used to work before 8ff866b (Add repaint-mode bind function, 2019-04-01),
and the optimization added by that commit will still work.
Actually before that commit we didn't execute the mode prompt if both left and right prompts were empty.
But that seems like an accident from 767742c (Rework how the mode is reported in fish_vi_mode, 2015-06-04).
src/reader.rs
Outdated
| let exec_right = !self.conf.right_prompt_cmd.is_empty() | ||
| && (self.conf.right_prompt_cmd != RIGHT_PROMPT_FUNCTION_NAME || { | ||
| // Load fish_prompt.fish to ensure we don't miss fish_right_prompt if it is defined there. | ||
| function::load(LEFT_PROMPT_FUNCTION_NAME, self.parser); |
There was a problem hiding this comment.
In which scenario is this necessary?
I'd think the simpler
let exec_right = !self.conf.right_prompt_cmd.is_empty();
would be enough because we'll execute the left prompt before the
right one; by that time the left prompt should be defined.
Maybe it's in case left_prompt_cmd is empty? This command seems
to work even if .config/fish/functions/fish_prompt.fish defines both
left and right prompts.
HOME=$PWD target/debug/fish -C 'read -p "" -R fish_right_prompt'
I'm not against being explicit about the fact that we support putting
both prompts in fish_prompt.fish but I'm not sure if this code is
actually used.
There was a problem hiding this comment.
HOME=$PWD target/debug/fish -C 'read -p "" -R fish_right_prompt'
Doesn't show the right prompt for me. Maybe something else was sourcing fish_prompt.fish for you?
This also applies to fish_mode_prompt (which additionally doesn't render for the first commandline). These functions can't be used from scripts either.
To properly support putting prompt functions in fish_prompt.fish, I think we'd need to load it at throwing_main.
I haven't added this to the pull request because I'm unsure if we actually need to support this.
src/reader.rs
Outdated
| let r_len = self.conf.right_prompt_cmd.len(); | ||
|
|
||
| let exec_left = l_len != 0 && function::exists(&self.conf.left_prompt_cmd, self.parser); | ||
| let exec_right = r_len != 0 && function::exists(&self.conf.right_prompt_cmd, self.parser); |
There was a problem hiding this comment.
this makes me wonder if there is a benefit to executing the prompt if exec_left and exec_left are both false.
I guess it means we'll execute the mode prompt. Seems fine - less surprising than returning early here I guess.
10000
src/screen.rs
| .map_or(1, |p| calc_prompt_lines(p)); | ||
| self.actual.cursor.y += prompt_line_count.checked_sub(1).unwrap(); | ||
| self.actual_left_prompt = None; | ||
| self.need_clear_screen = true; |
There was a problem hiding this comment.
Probably the line-losing prompt example is fixed by this line and the
zelf.screen.reset_line(true); addition?
Would be be nice to put them in a separate commit (after the code-moving commit),
else it's only easily visible when ignoring whitespace.
I.e. rewrite this commit with:
echo -n "\
diff --git a/src/reader.rs b/src/reader.rs
--- a/src/reader.rs
+++ b/src/reader.rs
@@ -4572,7 +4572,6 @@
false,
);
zelf.left_prompt_buff = join_strings(&prompt_list, '\n');
- zelf.screen.reset_line(true);
}
if exec_right {
diff --git a/src/screen.rs b/src/screen.rs
index 0f48737857..2f368cee2f 100644
--- a/src/screen.rs
+++ b/src/screen.rs
@@ -547,7 +547,6 @@
.map_or(1, |p| calc_prompt_lines(p));
self.actual.cursor.y += prompt_line_count.checked_sub(1).unwrap();
self.actual_left_prompt = None;
- self.need_clear_screen = true;
}
self.actual.resize(0);
self.need_clear_lines = true;
" | git apply --cached
doc_src/prompt.rst
Outdated
|
|
||
| set -g fish_transient_prompt 1 | ||
|
|
||
| With this set, fish re-runs ``fish_prompt`` (and ``fish_right_prompt``) with the ``--final-rendering`` argument before pushing commandline to scrollback. We can use this to display only the current directory name in the final prompt:: |
There was a problem hiding this comment.
prompt.rst is a tutorial document, try to keep this language simple.
"With this set, fish re-runs fish_prompt and fish_right_prompt with a --final-rendering argument before running the next commandline.
So you can use it to declutter your old prompts. For example if you want to see only the current directory name when you scroll up:
doc_src/tutorial.rst
Outdated
| :purple:`02/06/13` | ||
| :red:`/home/tutorial >` _ | ||
|
|
||
| You can use transient prompt functionality by setting ``fish_transient_prompt`` to 1 and checking for ``--final-rendering`` argument:: |
There was a problem hiding this comment.
This should explain what a "transient prompt" is.
I think this part is a better fit for interactive.rst (but tutorial.rst could do with a complete rewrite to be more targeted)
There was a problem hiding this comment.
I found an example explanation at https://github.com/romkatv/powerlevel10k#transient-prompt
There was a problem hiding this comment.
I don't think that example helps a lot because it's pretty specific to p10k's use of it. How about something like
"You can make your prompt transient by setting fish_transient_prompt to 1. That means fish will redraw the prompt before running a commandline, so you can change it then. Usually this is used to trim it down to declutter your terminal's scrollback history. When it runs the prompt again, it will pass a --final-rendering argument, so use that to distinguish."
|
i tried this feature and it works perfectly but also need to support fish_mode_prompt got output: [I] ❯ a
[I] ❯ a
[I] ❯ a
[I] ❯ xx a
fish: Unknown command: xx
[I] ❯ a
[I] ~/D/R/fish-shell/build (empty) [127]> bwant output(support fish_mode_prompt): # no mode
❯ a
❯ a
❯ a
❯ xx a
fish: Unknown command: xx
❯ a
[I] ~/D/R/fish-shell/build (empty) [127]> bmy example function fish_prompt --description 'Write out the prompt'
set -l last_pipestatus $pipestatus
set -lx __fish_last_status $status # Export for __fish_print_pipestatus.
if contains -- --final-rendering $argv
__transient_prompt_func $last_pipestatus
return 0
end
set -l normal (set_color normal)
set -q fish_color_status
or set -g fish_color_status red
# Color the prompt differently when we're root
set -l color_cwd $fish_color_cwd
set -l suffix '>'
if functions -q fish_is_root_user; and fish_is_root_user
if set -q fish_color_cwd_root
set color_cwd $fish_color_cwd_root
end
set suffix '#'
end
# Write pipestatus
# If the status was carried over (if no command is issued or if `set` leaves the status untouched), don't bold it.
set -l bold_flag --bold
set -q __fish_prompt_status_generation; or set -g __fish_prompt_status_generation $status_generation
if test $__fish_prompt_status_generation = $status_generation
set bold_flag
end
set __fish_prompt_status_generation $status_generation
set -l status_color (set_color $fish_color_status)
set -l statusb_color (set_color $bold_flag $fish_color_status)
set -l prompt_status (__fish_print_pipestatus "[" "]" "|" "$status_color" "$statusb_color" $last_pipestatus)
# echo -n -s (_prompt_login)' ' (set_color $color_cwd) (prompt_pwd) $normal (fish_vcs_prompt) $normal " "$prompt_status $suffix " "
set -l _prompt_pwd (prompt_pwd -D2 -d1)
set -l _prompt_pwd_parts (string split / $_prompt_pwd)
set _prompt_pwd_parts[-1] (set_color -o)$_prompt_pwd_parts[-1](set_color normal)
set _prompt_pwd (string join / $_prompt_pwd_parts)
echo -n -s (set_color $color_cwd) $_prompt_pwd $normal (fish_vcs_prompt) $normal " "$prompt_status $suffix " "
end
function fish_mode_prompt --description 'Displays the current mode'
# not support in fish_mode_prompt
if contains -- --final-rendering $argv
return 0
end
# To reuse the mode indicator use this function instead
fish_default_mode_prompt
end
# test right prompt
function fish_right_prompt
if contains -- --final-rendering $argv
echo "a"
return 0
end
echo "b"
end
function __transient_prompt_func
set --local color green
if test $argv[-1] -ne 0
set color red
end
set --local _msg "pwd="(prompt_pwd -d0) "date="(date +"%Y-%m-%d %H:%M:%S")" "(_cmd_duration)
set --local _symbol (set_color $color)"❯"(set_color normal)
set --local _prompt "\033]8;;$_msg\033\\$_symbol\033]8;;\033\\"
echo -e $_prompt" "
end
function _cmd_duration
test $CMD_DURATION -lt 100 && echo "" && return 0
set --local secs (math --scale=1 $CMD_DURATION/1000 % 60)
set --local mins (math --scale=0 $CMD_DURATION/60000 % 60)
set --local hours (math --scale=0 $CMD_DURATION/3600000)
set --local out
test $hours -gt 0 && set --local --append out $hours"h"
test $mins -gt 0 && set --local --append out $mins"m"
test $secs -gt 0 && set --local --append out $secs"s"
echo " took="$out
end |
Also fixes repaint-mode being run interactively
Now, the right prompt will not be executed only if it is undefined `fish_right_prompt`. This allows the use of `read -p 'echo left' -R 'echo right'`. Also changes condition for use of `DEFAULT_PROMPT` for consistency.
If you use `set t 10; function fish_prompt; seq $t; set t $(math $t - 10); end` and trigger a repaint, you will see 9 residual lines from the previous prompt.
|
Sorry for the delay. |
|
Only compiles with rustc >= 1.79.0. |
Our MSRV is 1.70, so yeah, this should be fixed for that. |
Before the final rendering, fish will now check if the `fish_transient_prompt` environment variable is enabled. Based on that, it will reexecute prompt functions with the `--final-rendering` argument.
Hide mode indicator in `fish_mode_prompt` and pipestatus in `fish_prompt` on final rendering.
| The output of the former is displayed on the left and the latter's output on the right side of the terminal. | ||
| For :ref:`vi mode <vi-mode>`, the output of :doc:`fish_mode_prompt <cmds/fish_mode_prompt>` will be prepended on the left. | ||
|
|
||
| If ``fish_transient_prompt`` is set to 1, fish will redraw the prompt with a ``--final-rendering`` argument before running a commandline, allowing you to change it before pushing it to the scrollback. |
There was a problem hiding this comment.
this should be :envvar:`fish_transient_prompt`, linking to an entry in language.rst.
I'll push some doc changes, LMK if you want it changed
| You can make your prompt transient by setting ``fish_transient_prompt`` to 1. That means fish will redraw the prompt before running a commandline, so you can change it then. Usually this is used to trim it down to declutter your terminal's scrollback history. When it runs the prompt again, it will pass a ``--final-rendering`` argument, so check for this to distinguish:: | ||
|
|
||
| function fish_prompt | ||
| if contains -- --final-rendering $argv |
There was a problem hiding this comment.
not sure if this should be part of the tutorial, which only contains very basic info.
We already have it on a separate "Writing your own prompt" page, we can maybe link to that from here.
| set bold_flag | ||
| # Write pipestatus if prompt is transient or transient prompt is disabled. | ||
| set -l prompt_status | ||
| if ! contains -- --final-rendering $argv |
There was a problem hiding this comment.
hmm the fact that this make the cursor jump to the left seems a bit jarring, so I'm not 100% sure if we want this. Leaving it for a separate PR
| .map_or(1, |p| calc_prompt_lines(p)); | ||
| self.actual.cursor.y += prompt_line_count.checked_sub(1).unwrap(); | ||
| self.actual_left_prompt = None; | ||
| self.need_clear_screen = true; |
There was a problem hiding this comment.
This seems fine but I guess always clearing to the end of screen is not always optimal:
If a background process does something like
sh -c 'sleep .1 && printf "\033[5Becho hello\033[5A"' &
it seems better to not hide that (the user can always press ctrl-l).
So in future we should maybe only clear the lines we painted, or only clear if the prompt actually shrinked.
|
ugh I accidentally overwrote the commit message. Also I'm seeing an issue where enter doesn't move to the next line (seems like an off-by-one-error). HOME=(mktemp -d) fish -C 'set -g fish_transient_prompt 1
function fish_prompt
echo \'$ \'
end'``` |
|
🤦 Forgot to test this without multiline prompt. The issue originates from 7acc2b7 which broke #6826. Fix: |
|
Thanks that works but I wonder why we need to reset the line (e.g. emit Tested with this prompt. Maybe there is an issue with multi-line prompts/commandlines or shrinking prompts but I'm not sure why. functions --copy fish_prompt default_prompt
function fish_prompt
set -l p "$(default_prompt)"
if contains -- --final-rendering $argv
printf '$ '
else
printf %s $p
end
enddiff --git a/src/reader.rs b/src/reader.rs
index 0531080d0b..3afcc644c8 100644
--- a/src/reader.rs
+++ b/src/reader.rs
@@ -4584,10 +4584,6 @@
self.left_prompt_buff =
join_strings(&self.exec_prompt_cmd(prompt_cmd, final_prompt), '\n');
-
- if final_prompt {
- self.screen.reset_line(true)
- }
}
// Don't execute the right prompt if it is undefined fish_right_prompt
diff --git a/src/screen.rs b/src/screen.rs
index dc0e7b8106..122b314bec 100644
--- a/src/screen.rs
+++ b/src/screen.rs
@@ -752,11 +752,18 @@
/// Return whether we believe the cursor is wrapped onto the last line, and that line is
/// otherwise empty. This includes both soft and hard wrapping.
pub fn cursor_is_wrapped_to_own_line(&self) -> bool {
- // Note == comparison against the line count is correct: we do not create a line just for the
- // cursor. If there is a line containing the cursor, then it means that line has contents and we
- // should return false.
// Don't consider dumb terminals to have wrapping for the purposes of this function.
- self.actual.cursor.x == 0 && self.actual.cursor.y == self.actual.line_count() && !is_dumb()
+ let old_res = self.actual.cursor.x == 0
+ && self.actual.cursor.y == self.actual.line_count()
+ && !is_dumb();
+ let newres = self.actual.cursor.x == 0
+ && self.actual.cursor.y != 0
+ && self.actual.cursor.y + 1 == self.actual.line_count()
+ && !is_dumb();
+ if old_res != newres {
+ panic!();
+ }
+ newres
}
/// Appends a character to the end of the line that the output cursor is on. This function
diff --git a/tests/checks/tmux-transient.fish b/tests/checks/tmux-transient.fish
index c595fbe601..3371e94f3d 100644
--- a/tests/checks/tmux-transient.fish
+++ b/tests/checks/tmux-transient.fish
@@ -10,14 +10,28 @@
printf "> full prompt > "
end
end
- bind enter "set transient true; commandline -f repaint execute"
+ bind ctrl-j "set transient true; commandline -f repaint execute"
'
isolated-tmux-start
-isolated-tmux send-keys 'echo foo' Enter
+isolated-tmux send-keys 'echo foo' C-j
tmux-sleep
isolated-tmux capture-pane -p
# CHECK: > echo foo
# CHECK: foo
# CHECK: > full prompt >
+
+# Regression test for transient prompt with single-line prompts.
+isolated-tmux send-keys C-u '
+ set -g fish_transient_prompt 1
+ function fish_prompt
+ printf "\$ "
+ end
+' C-l
+isolated-tmux send-keys Enter Enter
+tmux-sleep
+isolated-tmux capture-pane -p
+# CHECK: $
+# CHECK: $
+# CHECK: $BTW I wonder if we should remove the "transient" from the variable name, to make the connection to the fish prompt argument more obvious: set -g fish_prompt_repaint_preexec
# calls "fish_prompt --preexec" |
Without this, multiline prompts will show duplicated lines. diff --git a/src/reader.rs b/src/reader.rs
index 3ac0fc9b2..50a26333d 100644
--- a/src/reader.rs
+++ b/src/reader.rs
@@ -4588,7 +4588,7 @@ fn exec_prompt(&mut self, full_prompt: bool, final_prompt: bool) {
join_strings(&self.exec_prompt_cmd(prompt_cmd, final_prompt), '\n');
if final_prompt {
- self.screen.reset_line(true)
+ self.screen.multiline_prompt_hack();
}
}
diff --git a/src/screen.rs b/src/screen.rs
index fb26d2c2d..d61208b13 100644
--- a/src/screen.rs
+++ b/src/screen.rs
@@ -528,6 +528,18 @@ struct ScrolledCursor {
self.save_status();
}
+ pub fn multiline_prompt_hack(&mut self) {
+ // If the prompt is multi-line, we need to move up to the prompt's initial line. We do this
+ // by lying to ourselves and claiming that we're really below what we consider "line 0"
+ // (which is the last line of the prompt). This will cause us to move up to try to get back
+ // to line 0, but really we're getting back to the initial line of the prompt.
+ let prompt_line_count = self
+ .actual_left_prompt
+ .as_ref()
+ .map_or(1, |p| calc_prompt_lines(p));
+ self.actual.cursor.y += prompt_line_count.checked_sub(1).unwrap();
+ }
+
/// Resets the screen buffer's internal knowledge about the contents of the screen,
/// optionally repainting the prompt as well.
/// This function assumes that the current line is still valid.
@@ -539,15 +551,7 @@ pub fn reset_line(&mut self, repaint_prompt: bool /* = false */) {
std::cmp::max(self.actual_lines_before_reset, self.actual.line_count());
if repaint_prompt {
- // If the prompt is multi-line, we need to move up to the prompt's initial line. We do this
- // by lying to ourselves and claiming that we're really below what we consider "line 0"
- // (which is the last line of the prompt). This will cause us to move up to try to get back
- // to line 0, but really we're getting back to the initial line of the prompt.
- let prompt_line_count = self
- .actual_left_prompt
- .as_ref()
- .map_or(1, |p| calc_prompt_lines(p));
- self.actual.cursor.y += prompt_line_count.checked_sub(1).unwrap();
+ self.multiline_prompt_hack();
self.actual_left_prompt = None;
self.need_clear_screen = true;
}
I think we should keep it for better searchability. Also, |
|
On Mon, Apr 14, 2025 at 02:16:12AM -0700, kerty0 wrote:
kerty0 left a comment (fish-shell/fish-shell#11153)
> Thanks that works but I wonder why we need to reset the line (e.g. emit \r here).
Without this, multiline prompts will show duplicated lines.
The core issue is that fish doesn't handle multiline prompts as part
of the screen. I have an almost-working fix for proper multiline
prompt handling, but this hack should also work:
I see, that makes sense.
Extending the hack is reasonable.
If you need more time, I'll apply this.
Your other diff about `cursor_is_wrapped_to_own_line()` might be
useful for empty prompts, I'm not sure.
> BTW I wonder if we should remove the "transient" from the variable name
I think we should keep it for better searchability. Also, `preexec`
wouldn't be accurate since we don't execute empty commandlines,
so this would cause confusion with the `fish_preexec` event.
Right. I noticed it's also not triggered for `alt-l`
or when `sleep 1 &` finishes (both use the poorly-named __fish_echo).
We should probably trigger the transient prompt here, maybe with yet
another repaint command:
diff --git a/share/functions/__fish_echo.fish b/share/functions/__fish_echo.fish
index 94768fbdc1..3f7eac8ab0 100644
--- a/share/functions/__fish_echo.fish
+++ b/share/functions/__fish_echo.fish
@@ -4,5 +4,5 @@
$argv >&2
string >&2 repeat -N \n --count=(math (count (fish_prompt)) - 1)
string >&2 repeat -N \n --count=(math $line - 1)
- commandline -f repaint
+ commandline -f final-repaint
end
|
The
Is commandline duplication there actually desirable? We could modify function __fish_echo
set -l offset (math (fish_prompt | count) + (commandline --line) - 2)
if [ $offset -gt 0 ]
echo -n \e\[{$offset}A
end
echo -n \r\e\[0J
$argv >&2
string >&2 repeat -N \n --count=$offset
commandline -f repaint
endThis would change the behavior from: prompt 1> sleep 1 &
prompt 2>
fish: Job 1, 'sleep 1 &' has ended
prompt 2>To: prompt 1> sleep 1 &
fish: Job 1, 'sleep 1 &' has ended
prompt 2> |
|
On Tue, Apr 15, 2025 at 09:13:26AM -0700, kerty0 wrote:
kerty0 left a comment (fish-shell/fish-shell#11153)
> Your other diff about `cursor_is_wrapped_to_own_line()` might be useful for empty prompts, I'm not sure.
The `cursor_is_wrapped_to_own_line()` diff is necessary to fix the regression of #6826 introduced by 7acc2b7.
ok that sounds reasonable
> We should probably trigger the transient prompt here, maybe with yet another repaint command:
Is commandline duplication there actually desirable? We could modify `__fish_echo` to something like this:
yeah that seems better I think. It's currently broken for multiline commandlines.
For example
sleep 1 & commandline -i \{\n\{\n\{
Unrelated to this, there are lots of glitches with bindings like
alt-l that produce output. #11300 is one of them. Others are `ls
<tab><tab><alt-l>` or `ls <shift-tab><alt-l`.
I think we want a better design.
maybe things like alt-l should
1. completely clear the current prompt and commandline rendering
2. push a new commandline state
3. insert a new commandline and execute it, using the same machinery as other commands
3.1. somehow suppress drawing of the commandline+prompt, so we don't see (e.g. `$ ls -h`)
transient prompt gets us halfway there
4. pop the commandline state
5. redraw the old prompt and commandline
I'm not saying we shouldn't implement it in fish script but something like this is probably
easier to implement correctly at the source level
|
Extend a hack multi-line prompts to the new transient prompt code path. This fixes transient prompt with single-line prompts; added a test case. While at it, add a test that covers the need for this hack. Patch-by: kerty <g.kabakov@inbox.ru> See #11153 (comment)
Commit 7acc2b7 added an empty line to our screen representation if we are wrapped. This regressed the fix for #6826. In the attached test case, there is a spurious empty line after the first one. Adjust the fix to remove it again. Patch-by: kerty <g.kabakov@inbox.ru> #11153 (comment)
Added in fish 4.2.1: fish-shell/fish-shell#11153
Added in fish 4.2.1: fish-shell/fish-shell#11153
Description
Now, before the final redraw, fish will check if
fish_transient_promptorfish_right_transient_promptis defined. If either is defined, fish will redraw the commandline using the defined transient prompt functions instead of the normal ones.Additionally, if the built-in
readusesfish_promptorfish_right_prompt, it will be redrawn using the defined transient prompt functions.Fixes issue #11101
TODOs: