Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add forward-single-char input command #7217

Merged
merged 2 commits into from Aug 1, 2020
Merged

Add forward-single-char input command #7217

merged 2 commits into from Aug 1, 2020

Conversation

PoignardAzur
Copy link
Contributor

Resolves #4984

Description

There are two ways to solve the above issue: change forward-char to only autocomplete one character or add a new input function that autocompletes one character.

Downsides of changing forward-char:

  • Changes existing behavior, which might annoy users.
  • While power users may or may not know to use the end key to get the previous behavior, naive users will typically keep the right arrow pressed, which is a slight disruption to their workflow.

Downsides of adding an input function:

  • Makes code slightly more complicated (one more function which has to be propagated through all keybindings files), in a way that is neither fundamental nor orthogonal.
  • Some code ends up copy-pasted with subtle differences.
  • Breaks symmetry in the code (eg readers might wonder "why isn't there a backward-single-char command?").

Downsides of the status quo (eg why I want this feature):

  • I often find myself in a situation where I want to keep only the first few characters of a line auto-completion is giving me, without doing a word jump; for instance, I'm looking for somecommand some/path and fish is suggesting somecommand someverylongsinglewordpath. In these cases, being able to pick only the part of the auto-completion that interests me would be a boon.

Ultimately, which solution we pick is a matter of philosophy. I implemented the first one (forward-char doesn't autocomplete everything), which I personally prefer because it better fits a natural progression with other keybindings: right-arrow autocompletes a character, ctrl+right-arrow autocompletes a word, end autocompletes the entire line.

Pulling from fish's design page:

Fish should be user friendly, but not at the expense of expressiveness. Most tradeoffs between power and ease of use can be avoided with careful design.

I think repurposing forward-char is more expressive. Naive users may not know how to use the more advanced binding (end key), but that can be mitigated with good documentation and changelog notes.

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.rst

@faho
Copy link
Member

faho commented Jul 21, 2020

While power users may or may not know to use the end key to get the previous behavior, naive users will typically keep the right arrow pressed, which is a slight disruption to their workflow.

That's quite the understatement.

My guess? This kills suggestions, as a feature, for the majority of users.

"If I press right arrow to accept one character, why don't I just enter it myself?"

"It already knows what I want to type, why does it make me do all this pressing" (or holding)

Some will move to other movements or rebind the key, some won't, some will stop using fish entirely because this was a bad first impression.

Whatever we do must preserve the default behavior. This is not negotiable.


Now, as far as I can see there are two things you can do:

  • Add a new function "forward-char-accept-one-char"

or

  • Let "forward-char" return a status when it reaches the end of the real line, so you can bind forward-char or accept-autosuggestion

Copy link
Member

@faho faho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't change the default

@PoignardAzur
Copy link
Contributor Author

That makes sense.

Let "forward-char" return a status when it reaches the end of the real line, so you can bind forward-char or accept-autosuggestion

To be clear, there is currently no or input command that I'm aware of. Are you suggesting I add one?

@faho
Copy link
Member

faho commented Jul 21, 2020

To be clear, there is currently no or input command that I'm aware of. Are you suggesting I add one?

That would only be consistent - there is an "and".

Just adding another function is also fine by me, to be clear. It's just changing the default that isn't.

@PoignardAzur
Copy link
Contributor Author

PoignardAzur commented Jul 22, 2020

Nevermind, the or intrisic exists. It's just not documented in the man page or in bind --function-names, even though and is. I might fix that in another commit.

@faho
Copy link
Member

faho commented Jul 22, 2020

Nevermind, the or intrisic exists.

It does not.

You're probably trying the fish keyword or - bind something something; or or bind something "something; or".

What is needed here is the bind function or. Check that code I linked, that's where the bind functions are implemented. There is no "func_or", there is a func_and.

(yes, I am aware the bind system is a teensy bit weird. It is what it is)

@PoignardAzur
Copy link
Contributor Author

PoignardAzur commented Jul 22, 2020

... ugh. I completely forgot that fish is also a command-line shell, with actual commands and stuff. Right.

(I did think it was weird there were so many undocumented intrinsics)

@PoignardAzur
Copy link
Contributor Author

PoignardAzur commented Jul 22, 2020

I'm changing keybindings and finalizing the PR. What's up with fish_vi_key_bindings.fish?

Eg what does this line:

bind -s --preset -m insert a forward-char repaint-mode

mean?

EDIT: Oh, they're vim keybindings. Duh.

@PoignardAzur PoignardAzur requested a review from faho July 22, 2020 22:05
Copy link
Member

@faho faho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't change the default behavior for ctrl-f.

share/functions/fish_default_key_bindings.fish Outdated Show resolved Hide resolved
share/functions/fish_vi_key_bindings.fish Outdated Show resolved Hide resolved
Copy link
Member

@krobelus krobelus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this contribution and the detailed description. We can't change the default as @faho said. Some inconsistency in the language/implementation is acceptable to support the new behavior.

Naive users may not know how to use the more advanced binding (end key), but that can be mitigated with good documentation and changelog notes.

I think most users do not, and should not need to read documentation. There is no excuse for not making the default bindings behave the way a naive user would expect.

src/input.cpp Show resolved Hide resolved
src/input.cpp Outdated Show resolved Hide resolved
src/reader.cpp Outdated Show resolved Hide resolved
This allows users to add custom keybindings to autocomplete only one
character at a time.

Resolves #4984
@PoignardAzur
Copy link
Contributor Author

PoignardAzur commented Jul 26, 2020

Done. On one hand, I'm a little mad because I knew from the beginning I should probably have done this, but on the other hand the successive refactors gave me a better understanding of the codebase, so I guess that's okay?

@PoignardAzur
Copy link
Contributor Author

@faho Any feedback?

@zanchey
Copy link
Member

zanchey commented Jul 30, 2020

Fabian's on holiday at present, so perhaps @krobelus or @ridiculousfish?

@PoignardAzur PoignardAzur changed the title Change foward-char to only advance auto-completion by one character Add forward-single-char input command Jul 30, 2020
@krobelus krobelus self-assigned this Jul 30, 2020
@ridiculousfish
Copy link
Member

Reviewed, latest changes look great to me. I will wait a while before merging in case krobelus has more comments.

@PoignardAzur
Copy link
Contributor Author

PoignardAzur commented Aug 1, 2020

Looking to add unit tests for the func_and and func_or commands.

Is there an input command that's both a noop and guaranteed to return true or false?

Copy link
Member

@krobelus krobelus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PoignardAzur LGTM, let's merge.

Of course you're more than welcome to add unit tests in a follow up!
I don't know much about the bind system, I imagine you can try to mock such readline commands.

@krobelus krobelus merged commit e7f0b58 into fish-shell:master Aug 1, 2020
@PoignardAzur
Copy link
Contributor Author

The way the mapping system works looks hard to mock.

Honestly, I get the sense that func_and and function statuses are barely used anyway. I'll just celebrate a PR merged and wait for the feature to reach my package manager.

Yay \o/

@PoignardAzur PoignardAzur deleted the forward_char_single branch August 1, 2020 11:30
@krobelus
Copy link
Member

krobelus commented Aug 1, 2020

Yeah. I added forward-single-char to the changelog, but not the new func_or since there is probably little use for it.

Anyway, thanks for the contribution! Note that there are also nightly builds, or you can just make install any development version.

@zanchey zanchey added this to the fish 3.2.0 milestone Sep 27, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Ctrl+F to complete one character at a time
5 participants