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.

Sign up for GitHub

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

Jump to bottom

Vertical navigation #543

Closed
wants to merge 3 commits into from
Closed

Vertical navigation #543

wants to merge 3 commits into from

Conversation

LEOYoon-Tsaw
Copy link
Member

@LEOYoon-Tsaw LEOYoon-Tsaw commented May 1, 2022 •
edited

Pull request

Issue tracker

Fixes will automatically close the related issue

Fixes #

Feature

Describe feature of pull request

Unit test

  • Done

Manual test

  • Done

Code Review

  1. Unit and manual test pass
  2. GitHub Action CI pass
  3. At least one contributor reviews and votes
  4. Can be merged clean without conflicts
  5. PR will be merged by rebase upstream base

Additional Info

Reintroduce #397, see original discussion there

@LEOYoon-Tsaw
General support for navigating in vertical mode
8b69c09 
Add navigator awareness of vertical mode

(cherry picked from commit 4114f88)
@LEOYoon-Tsaw
selector shouldn't consum up and down entirely
8af7a01 
(cherry picked from commit dc84898)
@LEOYoon-Tsaw
A better solution to navigator, will not override user customization
0cd271c 
(cherry picked from commit f4ff86a)
@LEOYoon-Tsaw
Copy link
Member Author

One more point to add here:

We do not need to worry about that inline_preedit implies horizontal cursor movement despite in vertical mode. Since there's no way we can accommodate both horizontal cursor movement and vertical candidate selection.

For example, when the user hit left arrow, does they want to move cursor back or move to the next candidate? This is ambiguous. What we can do is to assume everything is vertical in vertical mode, and keep the logic intact.

@nasyxx
Copy link

nasyxx commented Oct 12, 2022

快两年过去了，这个还有机会合并吗？

lotem added a commit to lotem/librime that referenced this pull request Jan 23, 2023
@lotem
feat(navigator,selector): vertical navigation
7b9d33c 
refactor KeyBindingProcessor:
make KeyBindingProcessor::Handler return bool; return false to fall back
to the next processor;
include mutiple `Keymap`s for defining key bindings for each
(Horizontal, Vertical) x (Stacked, Linear) combination.

closes rime#543
@lotem lotem mentioned this pull request Jan 23, 2023
Merged
2 tasks
@lotem
Copy link
Member

lotem commented Jan 23, 2023 •
edited

I created another PR #603 which implements the feature using KeyBindingProcessor.

The code in the new PR is easier to understand than mapping key codes, IMHO. Please take a look.

@LEOYoon-Tsaw
Copy link
Member Author

Thank you, this sounds very exciting! I'll give it a try and put feedbacks there

2 similar comments
@LEOYoon-Tsaw
Copy link
Member Author

Thank you, this sounds very exciting! I'll give it a try and put feedbacks there

@LEOYoon-Tsaw
Copy link
Member Author

Thank you, this sounds very exciting! I'll give it a try and put feedbacks there

@lotem lotem closed this in d79f6b3 Jan 24, 2023
@LEOYoon-Tsaw LEOYoon-Tsaw deleted the vertical_navigation branch January 24, 2023 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@LEOYoon-Tsaw @nasyxx @lotem