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

Improve pager wraparound #4680

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@faho
Copy link
Member

faho commented Jan 23, 2018

Description

This fixes a few cases where the pager didn't correctly wrap around.

It fixes #4669, in that moving down now also wraps around if the current selection is the last row in the last column. The same goes for moving "east" (e.g. via "forward-char") and "west" (e.g. via "backward-char").

It also fixes #3115, by allowing up to move into the pager if nothing is selected. So up from the first element will still jump into the commandline (which we might want to change, since it's the only way to do so), while up from the commandline will jump to the last element in the pager.

TODOs:

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

faho added some commits Jan 23, 2018

[Pager] Make up without selected contents jump back into the pager
This makes it possible to select the last element of the completions
with just one keypress.

Fixes #3115.

@faho faho added this to the fish-3.0 milestone Jan 23, 2018

@laughedelic

This comment has been minimized.

Copy link
Contributor

laughedelic commented Jan 23, 2018

Cool 👍 Does it work the same with

  • tab/shift-tab,
  • / and
  • ctrl-n/ctrl-p?

@laughedelic laughedelic referenced this pull request Jan 23, 2018

Closed

Tab Completion #8

@faho

This comment has been minimized.

Copy link
Member

faho commented Jan 23, 2018

@laughedelic: Of course this is independent of the particular key - this modifies the pager's movement function, so any binding that calls any bind function that then moves the pager selection with one of the affected directions will be affected.

↓/↑ and
ctrl-n/ctrl-p?

These are bound to "up-or-search"/"down-or-search" respectively, which are wrapper functions that call the "up-line"/"down-line" bind functions. So yes, they are fully affected.

tab/shift-tab

These are bound to "complete" and "complete-and-search", which call the pager movement with "direction_next"/"direction_prev". They aren't affected, but there's nothing particularly wrong with them as far as I'm aware.

@laughedelic

This comment has been minimized.

Copy link
Contributor

laughedelic commented Jan 23, 2018

OK. Thanks for clarifying!

[Pager] Adjust tests for changes in behavior
Since moving west no longer gets stuck in the top row (but instead
wraps around to the bottom row), this needs to have some indices
changed.
@faho

This comment has been minimized.

Copy link
Member

faho commented Jan 24, 2018

This needs some adjustment to the tests - I didn't notice that we had some in src/fish_tests.cpp (I always forget about that, and I don't think ninja updated that correctly).

The first failure is moving "west". Let's say the pager looks like this:

[1] 2  3  4  5
6   7  8  9  A
B   C  D  E

(with [] denoting the cursor position on the 1)

and move "west"? Currently, this goes to 5, while moving "east" then goes to 6 (i.e. getting back to 1 from 5 requires either going east and then up or going west 4 times). With this, it goes to E instead (i.e. the last filled column in the last row). Moving east then returns one to 1.

So moving east or west just iterates in (western) reading-order - go through all the columns, then go to the next row and the starting column. Whereas now it does that, except it gets stuck in the last row.

This makes it so that east/west reverse each other, which is a nice property. On the other hand, that "column memory" feature is now gone as far as I can tell.

faho added some commits Jan 25, 2018

@faho

This comment has been minimized.

Copy link
Member

faho commented Jan 25, 2018

Merged as 1307991..12c249a.

I originally used github's conflict resolution feature, but that adds a merge commit, which is kinda superfluous when I could just directly change the changelog commit to resolve the commit.

@faho faho closed this Jan 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment