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

highlight candidate (selection without committance) #650

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
plugins/*/
build/
build-static/
debug/
Expand All @@ -14,3 +15,4 @@ node_modules/
.*.swp
.cache/
*.7z
*.bz2
14 changes: 14 additions & 0 deletions src/rime/context.cc
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,20 @@ bool Context::Select(size_t index) {
return false;
}

bool Context::Hilite(size_t index) {
Copy link
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.

if (composition_.empty())
return false;
Segment& seg(composition_.back());
if (auto cand = seg.GetCandidateAt(index)) {
seg.selected_index = index;
seg.tags.insert("paging");
DLOG(INFO) << "Hilited: '" << cand->text() << "', index = " << index;
update_notifier_(this);
return true;
}
return false;
}

bool Context::DeleteCandidate(
function<an<Candidate>(Segment& seg)> get_candidate) {
if (composition_.empty())
Expand Down
1 change: 1 addition & 0 deletions src/rime/context.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ class RIME_API Context {

// return false if there is no candidate at index
bool Select(size_t index);
bool Hilite(size_t index);
bool DeleteCandidate(size_t index);
// return false if there's no candidate for current segment
bool ConfirmCurrentSelection();
Expand Down
22 changes: 20 additions & 2 deletions src/rime/gear/selector.cc
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ inline static bool is_linear_layout(Context* ctx) {
}

ProcessResult Selector::ProcessKeyEvent(const KeyEvent& key_event) {
if (key_event.release() || key_event.alt() || key_event.super())
if (key_event.release() || key_event.super())
return kNoop;
Context* ctx = engine_->context();
if (ctx->composition().empty())
Expand Down Expand Up @@ -149,7 +149,8 @@ ProcessResult Selector::ProcessKeyEvent(const KeyEvent& key_event) {
else if (ch >= XK_KP_0 && ch <= XK_KP_9)
index = ((ch - XK_KP_0) + 9) % 10;
if (index >= 0) {
SelectCandidateAt(ctx, index);
key_event.alt() ? HiliteCandidateAt(ctx, index)
Copy link
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
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.

: SelectCandidateAt(ctx, index);
return kAccepted;
}
// not handled
Expand Down Expand Up @@ -252,6 +253,23 @@ bool Selector::End(Context* ctx) {
return Home(ctx);
}

bool Selector::HiliteCandidateAt(Context* ctx, int index) {
Composition& comp = ctx->composition();
if (comp.empty())
return false;
int page_size = engine_->schema()->page_size();
if (index >= page_size)
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
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.

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
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
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 .).

return true;
}

bool Selector::SelectCandidateAt(Context* ctx, int index) {
Composition& comp = ctx->composition();
if (comp.empty())
Expand Down
1 change: 1 addition & 0 deletions src/rime/gear/selector.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ class Selector : public Processor, public KeyBindingProcessor<Selector, 4> {
Handler Home;
Handler End;

bool HiliteCandidateAt(Context* ctx, int index);
bool SelectCandidateAt(Context* ctx, int index);
};

Expand Down
11 changes: 11 additions & 0 deletions src/rime_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1038,6 +1038,15 @@ RIME_API Bool RimeSelectCandidateOnCurrentPage(RimeSessionId session_id,
return do_with_candidate_on_current_page(session_id, index, &Context::Select);
}

RIME_API Bool RimeHiliteCandidate(RimeSessionId session_id, size_t index) {
return do_with_candidate(session_id, index, &Context::Hilite);
}

RIME_API Bool RimeHiliteCandidateOnCurrentPage(RimeSessionId session_id,
size_t index) {
return do_with_candidate_on_current_page(session_id, index, &Context::Hilite);
}

const char* RimeGetVersion() {
return RIME_VERSION;
}
Expand Down Expand Up @@ -1185,6 +1194,8 @@ RIME_API RimeApi* rime_get_api() {
s_api.context_proto = nullptr;
s_api.status_proto = nullptr;
s_api.get_state_label = &RimeGetStateLabel;
s_api.hilite_candidate = &RimeHiliteCandidate;
s_api.hilite_candidate_on_current_page = &RimeHiliteCandidateOnCurrentPage;
s_api.delete_candidate = &RimeDeleteCandidate;
s_api.delete_candidate_on_current_page = &RimeDeleteCandidateOnCurrentPage;
s_api.get_state_label_abbreviated = &RimeGetStateLabelAbbreviated;
Expand Down
7 changes: 7 additions & 0 deletions src/rime_api.h
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,9 @@ RIME_API Bool RimeCandidateListFromIndex(RimeSessionId session_id,
RIME_API Bool RimeSelectCandidate(RimeSessionId session_id, size_t index);
RIME_API Bool RimeSelectCandidateOnCurrentPage(RimeSessionId session_id,
size_t index);
RIME_API Bool RimeHiliteCandidate(RimeSessionId session_id, size_t index);
RIME_API Bool RimeHiliteCandidateOnCurrentPage(RimeSessionId session_id,
size_t index);
RIME_API Bool RimeDeleteCandidate(RimeSessionId session_id, size_t index);
RIME_API Bool RimeDeleteCandidateOnCurrentPage(RimeSessionId session_id,
size_t index);
Expand Down Expand Up @@ -635,6 +638,10 @@ typedef struct rime_api_t {
const char* option_name,
Bool state);

Bool (*hilite_candidate)(RimeSessionId session_id, size_t index);
Copy link
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.

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

//! delete a candidate at the given index in candidate list.
Bool (*delete_candidate)(RimeSessionId session_id, size_t index);
//! delete a candidate from current page.
Expand Down