Skip to content

highlight candidate (selection without committance) - #650

Closed
groverlynn wants to merge 5 commits into
rime:masterfrom
groverlynn:choose
Closed

highlight candidate (selection without committance)#650
groverlynn wants to merge 5 commits into
rime:masterfrom
groverlynn:choose

Conversation

@groverlynn

Copy link
Copy Markdown
Contributor

Pull request

Allow users to choose a candidate -- selection without committance, serving as shortcuts for select by multiple consecutive navigators. By default, this also enables Alt + select_key as choose whereas select_key remains as select.

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

@MokOopsing

Copy link
Copy Markdown
Contributor

你需要的是fluid_editor吧?

@groverlynn

Copy link
Copy Markdown
Contributor Author

你需要的是fluid_editor吧?

No. It's for mouse hoover.

@MokOopsing

Copy link
Copy Markdown
Contributor

#620

@groverlynn
groverlynn force-pushed the choose branch 2 times, most recently from a30967e to b05e719 Compare July 9, 2023 10:12
@groverlynn

groverlynn commented Jul 16, 2023

Copy link
Copy Markdown
Contributor Author

#620

peek is a horrible function name choice…
First, this so-called peek function does nothing like peeking in reality. Second and third, it does nothing like peek functions already used in librime, creating confusion on the top.
It also behave inconsistently with arrow_key selection (e.g. failing to set "paging" tag)…

@groverlynn groverlynn changed the title choose candidate (selection without committance) highlight candidate (selection without committance) Dec 20, 2023
Comment thread src/rime/gear/selector.cc
return false;
int selected_index = comp.back().selected_index;
int page_start = (selected_index / page_size) * page_size;
// hilite an already hilited candidate -> select this candidate

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's quite hidden logic. The function should do exactly what the name implies.

Comment thread src/rime/gear/selector.cc
if (page_start + index == selected_index)
return ctx->Select(selected_index);
comp.back().selected_index = page_start + index;
comp.back().tags.insert("paging");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Highlighting a candidate does not necessarily change page.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Highlighting a candidate does not necessarily change page.

But this is consistent with the behavior of navigators—even if there's no turning pages, user is no longer actively composing which means paging keys should take priority over composing keys (e.g. in the case of , and .).

Comment thread src/rime/gear/selector.cc
index = ((ch - XK_KP_0) + 9) % 10;
if (index >= 0) {
SelectCandidateAt(ctx, index);
key_event.alt() ? HiliteCandidateAt(ctx, index)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not in favour of this feature. Why would user want to do that with a key combo?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not in favour of this feature. Why would user want to do that with a key combo?

because, when intended to be followed by deleting the candidate, it's faster to use a key combo to jump from highlighting candidate 1 to highlighting candidate 10 than press down/right key 9 times, provided that user does not want to use mouse.

Comment thread src/rime/context.cc
return false;
}

bool Context::Hilite(size_t index) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's spell it Highlight.

It's one of the option I suggested in Leo's PR.
I would probably took his PR with some modifications.

Comment thread src/rime_api.h
const char* option_name,
Bool state);

Bool (*hilite_candidate)(RimeSessionId session_id, size_t index);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Inserting new members breaks ABI compatibility.
Don't.
Append new members to the end instead.

@groverlynn groverlynn closed this Feb 10, 2024
@groverlynn
groverlynn deleted the choose branch February 10, 2024 08:40
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