Add forward-char-passive

[mqudsi]

Add a forward-char-passive binding. (Better suggestions for the name are being accepted!)

The idea is to add a "non-destructive" version of forward-char that strictly moves but does not change the "meta state" of the shell (does not accept auto completion, does not change completion pager selection). This has been requested by a terminal emulator developer and it makes sense to me at a high (enough) level.

I'm not going to pretend to be aware of where each binding is used, but it seems to me that ideally instead of adding something new to the existing forward-char and forward-single-char bindings, it would make more sense for one of them (forward-single-char) to not automatically accept suggestions. It seems forward-single-char is primarily used in the vi key bindings, and it isn't especially clear to me that accepting the autosuggestion makes sense there. But out of an abundance of precaution and to avoid breaking things, I am adding this as a new keybinding altogether.

Fixes issue #10397

TODOs:

• Changes to fish usage are reflected in user documentation/manpages.
• Tests have been added for regressions fixed (test is confirmed to fail if forward-single-char is used instead)
• User-visible changes noted in CHANGELOG.rst

[kovidgoyal]

It would be nice if we had a pair of them for forward and back. back is not strictly necessary since as far as I know the regular backward-char doesnt do anything but that may not always remain true.

[krobelus]

Even more ideally, fish would add a function that has semantics of
"move cursor to the cell number X on line number Y, where Y is counted
from the line that starts the prompt".

The first seems eminently doable. The second might be a more difficult ask as things currently stand.

So the only difficulty is that move(X, Y) needs to potentially skip over Y-1 soft-wrapped lines?
We can probably use the current rendering if we want to do that.
If the current prompt rendering exceeds $LINES, does it still start from the prompt, or the first visible character?
Probably the first. Either way this scenario has always been somewhat broken in fish.

Anyway, making it work with something like forward-char seems more conventional here [*].

I'd expect that we'll eventually want a forward-char-like command that does not move in the pager and probably also not accept autosuggestion.
Crucially, when the pager is active, it should move the cursor inside the pager search field (which is impossible today).
This was suggested in #10268 (comment);
With the recent input event queueing changes this might be possible now.
Quick sketch:

bind \cf '
	if commandline --paging-mode
	    commandline -f move-right
	else if commandline --is-at-end
	    commandline -f accept-autosuggestion-char
	end
	    commandline -f forward-char # different name to avoid breakage?
	end
'

I'm not sure if the autosuggestion part makes sense.
In general, it's not clear where the line should be between scripts and builtin behavior.

[*]: Note that currently we use this to move to a X/Y coordinates (logical, ignoring soft-wrapping). Maybe it's worth making this more ergonomic.

    commandline -C 0
    for _line in (seq $line)[2..]
        commandline -f down-line
    end
    commandline -f beginning-of-line
    for _column in (seq $column)[2..]
        commandline -f forward-single-char
    end

[krobelus]

If we take forward-char-passive, I expect it should also move the cursor in paging mode (in the pager search field). I'm not sure what kitty integration expects.
Given that the forward-char naming is nicer for rebinding that the above if/else command,
it seems fine to have an overlapping low-level command. Perhaps we can find some kind of naming scheme for low-level commands.

[krobelus]

the first and last sleep are unnecessary because there is no relevant async work going on.
Also, I'd C-b instead of C-p would be less confusing

[kovidgoyal]

On Thu, Mar 28, 2024 at 12:45:22AM -0700, Johannes Altmanninger wrote: @krobelus commented on this pull request. > @@ -2511,6 +2511,16 @@ impl ReaderData { self.update_buff_pos(elt, Some(el.position() + 1)); } } + rl::ForwardCharPassive => { + let (elt, el) = self.active_edit_line(); + if self.is_navigating_pager_contents() { + // Do nothing If we take `forward-char-passive`, I expect it should also move the cursor in paging mode (in the pager search field). I'm not sure what kitty integration expects.

kitty shell integration only triggers if the cursor is at the prompt, not elsewhere. I dont use fish so I dont know what the pager is, but assuming it means the cursor is not at the shell prompt, kitty shell integration wont care.

[krobelus]

Yeah sounds good; FWIW the (completion) pager shows on Tab and the pager search field shows on Shift+Tab, Control+S or Control+R which move the cursor to that search field. Ideally we get rid of this search field but that's complicated

[kovidgoyal]

On Thu, Mar 28, 2024 at 12:57:46AM -0700, Johannes Altmanninger wrote: Yeah sounds good; FWIW the (completion) pager shows on Tab and the pager search field shows on Shift+Tab, Control+S or Control+R which move the cursor to that search field. Ideally we get rid of this search field but that's complicated

I tried it with the search field, its still detected as part of the prompt, so clicking does send arrow keys, which changes the selection in the list of matches. So kitty shell integration would require passive-forward/backward-char to move the cursor left or right in the search field instead of doing the same as left/right does now. Although, long term you should probably think about grabbing the mouse while in the pager to allow users to click on results to select. But that is irrelevant to this issue.

[krobelus]

I think this is good as-is, let's just add the backward variant and make them also move the cursor if self.pager.is_search_field_shown().

Would be nice to be able to use something like fish_autosuggestions_enabled=0 forward-char but that doesn't solve the fact that it moves in the pager.

Spitballing some more names: forward-char-nonmagic, forward-char-raw, forward-char-plain but none of those is clearly better.

[mqudsi]

I think if we also handle the self.pager.is_search_field_shown() case then it's the same thing as just globally updating the position if position != 0 (for backwards char) or if !self.is_at_end(el) (for forwards char)?

Or is it possible to have is_navigating_pager_contents() but not have is_search_field_shown() be true?

[krobelus]

Is it possible to have is_navigating_pager_contents() but not have is_search_field_shown() be true?

Yes. In that case we should probably do nothing (moving the cursor might break some invariants that completion insertion relies on)

Note there is another interesting case:
is_navigating_pager_contents() may be false but the pager might still be rendered so !pager.is_empty()
In that case forward-char already moves the cursor so we should match that.

[krobelus]

this is missing the backward variant

[mqudsi]

Good catch.

[mqudsi]

I think I'm going to update that match block (later) to remove the _ case so that that's a compilation error. _ shouldn't be left in there for cases where new items need to be manually classified.

[mqudsi]

The new logic is

                if !self.is_at_end(el) {
                    if elt == EditableLineTag::SearchField || !self.is_navigating_pager_contents() {
                        self.update_buff_pos(elt, Some(el.position() + 1));
                    }
                }

elt is SearchField only if both is_navigating_pager_contents() and is_search_field_shown() so this should match the logic you specified. If !is_navigating_pager_contents() then the cursor is moved regardless of whether the pager is empty, which should match the existing logic.

[mqudsi]

Other name suggestions:

• forward-char-cursor
• forward-char-cursor-only
• forward-cursor
• edit-cursor-right

But it's probably just bike shedding at this point.