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

Off-by-one error for df, dt, dF, dT motions with vi key bindings #5770

Closed
veryjos opened this issue Mar 26, 2019 · 5 comments · Fixed by #6329
Closed

Off-by-one error for df, dt, dF, dT motions with vi key bindings #5770

veryjos opened this issue Mar 26, 2019 · 5 comments · Fixed by #6329
Labels
bug Something that's not working as intended vi-mode
Milestone

Comments

@veryjos
Copy link

veryjos commented Mar 26, 2019

In vi, df and dF are inclusive (in that they delete the character the motion is targetting) and dt and dT are exclusive. Right now, in fish, all of these commands delete one character less than they do in vi.

To reproduce:

  1. enable fish_vi_key_bindings (fish_vi_key_bindings)
  2. write some text, attempt to use df, dF, dt, or dT and observe how it differs from vi
  • Reproduced with a fresh environment
  • fish --version: fish, version 3.0.2
  • uname -a: Linux josbox 4.20.15-1-MANJARO #1 SMP PREEMPT Sun Mar 10 08:50:44 UTC 2019 x86_64 GNU/Linux
veryjos added a commit to veryjos/fish-shell that referenced this issue Mar 26, 2019
Should fix fish-shell#5770.

Fixes an off-by-one error with df, dt, dF, and dT strokes. Too little
was deleted in fish as opposed to vi.
@faho faho added bug Something that's not working as intended vi-mode labels Mar 26, 2019
@faho faho added this to the fish-future milestone Mar 26, 2019
@faho
Copy link
Member

faho commented Mar 27, 2019

This is a more general problem because emacs' and vi's idea on where the cursor is differs.

See e.g. #4025, #4683.

Adding "forward-char" and "backward-char" to bindings doesn't really work because they'd also be executed if the command fails (e.g. do "dfG" on a line that doesn't have a "G").

At the very least it would have to be "forward-jump and backward-char" (and forward-jump would have to be adjusted to set the bind status).

(Yes, that is indeed just "and" without a semicolon - this isn't fish-script, it's the DSL for bind functions, which is a bit weird)

@veryjos
Copy link
Author

veryjos commented Mar 27, 2019

Have you guys considered using neovim as a backend to drive the shell prompt?

Is the plugin API robust enough to facilitate that? That's something I would be interested in working on- a plugin for fish that uses neovim as a backend to get proper vi bindings.

EDIT: Just for reference, that approach is first-class in Oni and an optional feature in VSCode's vim plugin.

@faho
Copy link
Member

faho commented Mar 27, 2019

Have you guys considered using neovim as a backend to drive the shell prompt?

It's been thrown around idly. I can't say I'm a fan of the dependency, and I'm not sure if it would actually work well, but if someone implemented it and it's well-done we might merge it.

@mqudsi
Copy link
Contributor

mqudsi commented Mar 29, 2019

I looked into it a few times myself, but the last time that I sat down to do it I was put off by the assumptions neovim makes about the purposes behind the integration. The RPC api is (was?) very restrictive in terms of how an integration must be approached and the kind of state that would be kept client-side, so I ended up tabling my efforts.

@rhogenson
Copy link
Contributor

This off-by-one inconsistency also applies to selecting text in visual mode: in vim, the selection includes the character under the cursor. In fish, the selection includes up to but not including the character under the cursor.

faho pushed a commit that referenced this issue Nov 19, 2019
The current cursor position should be included in the selection to be
consistent with the behavior of vi.

Fixes #5770
@zanchey zanchey modified the milestones: fish-future, fish 3.1.0 Nov 20, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something that's not working as intended vi-mode
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants