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 key bindings to jump to the next item of the same kind #45

Merged
merged 3 commits into from
Jun 17, 2024

Conversation

emesterhazy
Copy link
Collaborator

This PR does a few things:

  1. The keybindings for scroll up and scroll down are changed. After this PR the only bindings for this movement are ctrl-up and ctrl-down
  2. The keybindings for page up and page down (viewport) are changed. After this PR the only bindings for this movement are ctrl-page-up and ctrl-page-down.
  3. It adds new keybindings (page-up and page-down) to jump to the next item with the same type as the current item. For example, if the user is on a section header, page-down will jump to the next section header. If the user is on an editable line, page down will jump to the next editable line.

This works well, but probably needs a little bit of polish before submission. Hopefully this can unblock #39 which allows the user to collapse sections using the "focus outward" keybindings (i.e. the left arrow key).

Copy link
Owner

@arxanas arxanas left a comment

Choose a reason for hiding this comment

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

comment:

  • The scroll keybindings come directly from Vim (sorry, meant to comment that one of your other PRs but didn't have time), so I'd prefer to preserve them for now.
  • I'm okay with adding the more sane scrolling keybindings that you specified alongside them.
  • If you know any other interesting paradigms for these shortcuts (Emacs, Sublime Text, IntelliJ, VS Code, ...) I'm open to supporting them as well.

For now, I'm okay with changing page-up/-down to match Mercurial, but we may want to do a holistic reworking of all the keybindings at some point in the future.

src/ui.rs Outdated Show resolved Hide resolved
src/ui.rs Outdated Show resolved Hide resolved
src/ui.rs Show resolved Hide resolved
@emesterhazy
Copy link
Collaborator Author

Thanks for the feedback! Agreed that this should probably be refactored a bit, I just wanted to get something up as a proposal.

I'll be afk for the next week but when I get back I'll push a new revision.

@emesterhazy emesterhazy force-pushed the push-nlzzzsuuruwt branch 3 times, most recently from 2ade904 to d452041 Compare June 2, 2024 19:50
@emesterhazy
Copy link
Collaborator Author

Ok this is ready for another look. Thanks!

@arxanas arxanas self-requested a review June 15, 2024 22:36
Copy link
Owner

@arxanas arxanas left a comment

Choose a reason for hiding this comment

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

Looks good to me, just one tiny nit, you can merge this when done!

src/ui.rs Outdated Show resolved Hide resolved
@arxanas
Copy link
Owner

arxanas commented Jun 15, 2024

I queued #46 for merge; you might have to rebase/update tests.

This changes the bindings as follows:

For page up:

- Before: page-up OR ctrl-b
- After: ctrl-page-up OR ctrl-b

For page down:

- Before: page-down OR ctrl-f
- After: ctrl-page-down OR ctrl-f

The page-up and page-down keys will be repurposed in a subsequent commit to
jump between editable sections.
These new keybindings are added alongside the existing keybindings.

For scroll up:

- Before: ctrl-y
- After: ctrl-y OR ctrl-up

For scroll down:
- Before: ctrl-e
- After: ctrl-e OR ctrl-down
For example, if the currently highlighted row is a section header, the page
down key will jump to the next section. If the currently highlighted row is a
line change, the page down key will jump to the next selectable line.

The new keybindings are page-up and page-down.
@emesterhazy
Copy link
Collaborator Author

Thanks for reviewing!

@emesterhazy emesterhazy merged commit 4a836b4 into arxanas:main Jun 17, 2024
2 checks passed
@emesterhazy emesterhazy deleted the push-nlzzzsuuruwt branch June 17, 2024 15:32
@xzfc
Copy link

xzfc commented Sep 10, 2024

Nit: the corresponding "feature wishlist" item in the readme is still around.

- Jump to next/previous element of same kind.

@arxanas arxanas added this to the v0.4.0 milestone Oct 16, 2024
@arxanas
Copy link
Owner

arxanas commented Oct 25, 2024

@xzfc Thanks for pointing that out. I opened #74 to update README.md

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants