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

Fix default Alt+W keybinding #6110

Merged
merged 3 commits into from Oct 6, 2019
Merged

Fix default Alt+W keybinding #6110

merged 3 commits into from Oct 6, 2019

Conversation

gpanders
Copy link
Contributor

Description

The old keybinding would chop off the last line of the whatis output
when using a multi-line prompt. This fix corrects that.

Before

In this example, I am using the pure prompt, which is a multi-line prompt. Using whatis vim as an example, the correct output should be

~
❯ whatis vim
evim(1)                  - easy Vim, edit a file with Vim and setup for modeless editing
vim(1)                   - Vi IMproved, a programmer's text editor
vimdiff(1)               - edit two, three or four versions of a file with Vim and show differences
vimtutor(1)              - the Vim tutor

However, using the Alt+W keybinding shows

~
❯ vim| (<- Press Alt+W here)
evim(1)                  - easy Vim, edit a file with Vim and setup for modeless editing
vim(1)                   - Vi IMproved, a programmer's text editor
vimdiff(1)               - edit two, three or four versions of a file with Vim and show differences
~
❯ vim|

As you can see, the last line of the whatis output (vimtutor(1)) is missing.

After

~
❯ vim| (<- Press Alt+W here)
evim(1)                  - easy Vim, edit a file with Vim and setup for modeless editing
vim(1)                   - Vi IMproved, a programmer's text editor
vimdiff(1)               - edit two, three or four versions of a file with Vim and show differences
vimtutor(1)              - the Vim tutor
~
❯ vim|

This correctly shows the full output of whatis.

The old keybinding would chop off the last line of the `whatis` output
when using a multi-line prompt. This fix corrects that.
share/functions/__fish_whatis_current_token.fish Outdated Show resolved Hide resolved
whatis $tok[1]

set -l line_count (count (fish_prompt))
if test $line_count -gt 1
Copy link
Member

Choose a reason for hiding this comment

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

This if is not necessary I guess. Looks like __fish_list_current_token is using it as well, not sure why.

Copy link
Contributor Author

@gpanders gpanders Oct 8, 2019

Choose a reason for hiding this comment

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

I know this has already been merged, but this if is actually necessary:

When fish_prompt is a single line, this resolves to seq 2 1 which produces

2
1

which will end up printing two newlines! This is definitely not what we want (a single line prompt should print no extra newlines).

When fish_prompt is two lines, we get seq 2 2 which does indeed produce simply 2 and prints one extra newline, as desired.

The other solution offered below:

# Print enough newlines to have the prompt below the `whatis` output.
printf '%s\n' (fish_prompt)[1..-2]

seems to have the same issue.

If we really don't want an if check, we could use

for i in (seq (count (fish_prompt)) | tail -n+2)

though I'm not sure that's any better/cleaner/easier to read.

Copy link
Member

Choose a reason for hiding this comment

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

When fish_prompt is a single line, this resolves to seq 2 1 which produces

Aaaah! That's an incompatibility in seq implementations! GNU seq produces nothing in that case, while BSD seq does indeed produce 2 1.


if test $tok[1]
Copy link
Member

Choose a reason for hiding this comment

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

I don't know what's the best thing to do when there is no word at the cursor.
This looks fine, or we can return (and perhaps send an alert like __fish_man_page).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current implementation of Alt+W simply uses this if-check, so I'm in favor of leaving it how it is for now.

gpanders added a commit to gpanders/dotfiles that referenced this pull request Sep 18, 2019
@floam
Copy link
Member

floam commented Sep 21, 2019

Is there no simpler fix?

@krobelus
Copy link
Member

I realized that instead of

    set -l line_count (count (fish_prompt))
    for x in (seq 2 $line_count)
        printf "\n"
    end

we can do this

	# Print enough newlines to have the prompt below the `whatis` output.
	printf '%s\n' (fish_prompt)[1..-2]

(it seems like it doesn't matter what is printed on those lines because of commandline -f repaint)

Or do you mean simpler as in handled by fish itself?

__fish_list_current_token (bound to \el) does the same thing.

@faho faho added the bug Something that's not working as intended label Oct 6, 2019
@faho faho added this to the fish 3.1.0 milestone Oct 6, 2019
@faho faho merged commit e5fc8ab into fish-shell:master Oct 6, 2019
@faho
Copy link
Member

faho commented Oct 6, 2019

Merged, thanks, sorry for taking so long!

@gpanders
Copy link
Contributor Author

gpanders commented Oct 8, 2019

See this comment: #6110 (comment)

This pull request needs to be amended. I will open a new one right now.

faho pushed a commit that referenced this pull request Oct 8, 2019
Corrects #6110

BSD `seq` produces a down-counting sequence when the second argument is
smaller than the first, e.g.:

    $ seq 2 1
    2
    1
    $

While GNU `seq` produces no output at all:

    $ seq 2 1
    $

To accommodate for this behavior, only run `seq` when we are sure that
the second argument is greater than or equal to the first (in this case,
the second argument `line_count` should be greater than 1).
gpanders added a commit to gpanders/dotfiles that referenced this pull request Oct 11, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants