Skip to content

Vertical navigation#543

Closed
LEOYoon-Tsaw wants to merge 3 commits into
rime:masterfrom
LEOYoon-Tsaw:vertical_navigation
Closed

Vertical navigation#543
LEOYoon-Tsaw wants to merge 3 commits into
rime:masterfrom
LEOYoon-Tsaw:vertical_navigation

Conversation

@LEOYoon-Tsaw
Copy link
Copy Markdown
Member

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

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
Copy Markdown
Member Author

LEOYoon-Tsaw commented May 1, 2022

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
Copy Markdown

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
Copy Markdown
Member

lotem commented Jan 23, 2023 •
edited
Loading

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
Copy Markdown
Member Author

LEOYoon-Tsaw commented Jan 23, 2023

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

2 similar comments
@LEOYoon-Tsaw
Copy link
Copy Markdown
Member Author

LEOYoon-Tsaw commented Jan 23, 2023

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

@LEOYoon-Tsaw
Copy link
Copy Markdown
Member Author

LEOYoon-Tsaw commented Jan 23, 2023

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.

3 participants

@LEOYoon-Tsaw @nasyxx @lotem