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

feat: delete selected candidate #556

Closed
wants to merge 1 commit 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
29 changes: 21 additions & 8 deletions src/rime/context.cc
Original file line number Diff line number Diff line change
Expand Up @@ -123,19 +123,32 @@ bool Context::Select(size_t index) {
return false;
}

bool Context::DeleteCurrentSelection() {
bool Context::DeleteCandidate(function<an<Candidate> (Segment& seg)> get_candidate) {
Copy link
Member

Choose a reason for hiding this comment

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

This memeber function should have a definition in context.h, in the private: or pretected: section.

if (composition_.empty())
return false;
Segment& seg(composition_.back());
if (auto cand = seg.GetSelectedCandidate()) {
DLOG(INFO) << "Deleting: '" << cand->text()
<< "', selected_index = " << seg.selected_index;
delete_notifier_(this);
return true; // CAVEAT: this doesn't mean anything is deleted for sure
return false;
if (auto cand = get_candidate(seg)) {
DLOG(INFO) << "Deleting: '" << cand->text()
<< "', index = " << index;
delete_notifier_(this);
return true; // CAVEAT: this doesn't mean anything is deleted for sure
}
return false;
}

bool Context::DeleteCandidate(size_t index) {
return DeleteCandidate(
[index](Segment& seg) {
return seg.GetCandidateAt(index);
});
}

bool Context::DeleteCurrentSelection() {
return DeleteCandidate(
[](Segment& seg) {
return seg.GetSelectedCandidate();
});
}

bool Context::ConfirmCurrentSelection() {
if (composition_.empty())
return false;
Expand Down
1 change: 1 addition & 0 deletions src/rime/context.h
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ class Context {

// return false if there is no candidate at index
bool Select(size_t index);
bool DeleteCandidate(size_t index);
// return false if there's no candidate for current segment
bool ConfirmCurrentSelection();
bool DeleteCurrentSelection();
Expand Down
26 changes: 22 additions & 4 deletions src/rime_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1051,17 +1051,17 @@ size_t RimeGetCaretPos(RimeSessionId session_id) {
return ctx->caret_pos();
}

Bool RimeSelectCandidate(RimeSessionId session_id, size_t index) {
static bool do_with_candidate(RimeSessionId session_id, size_t index, bool (Context::* verb)(size_t index)) {
an<Session> session(Service::instance().GetSession(session_id));
if (!session)
return False;
Context *ctx = session->context();
if (!ctx)
return False;
return Bool(ctx->Select(index));
return Bool(ctx->*verb(index));
Copy link
Member

Choose a reason for hiding this comment

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

We need a pair of parentheses around the function part (ctx->*verb)(index).
Remove the Bool( ), because the return type is not Bool.

}

Bool RimeSelectCandidateOnCurrentPage(RimeSessionId session_id, size_t index) {
static bool do_with_candidate_on_current_page(RimeSessionId session_id, size_t index, bool (Context::* verb)(size_t index)) {
an<Session> session(Service::instance().GetSession(session_id));
if (!session)
return False;
Expand All @@ -1076,13 +1076,29 @@ Bool RimeSelectCandidateOnCurrentPage(RimeSessionId session_id, size_t index) {
return False;
const auto& seg(ctx->composition().back());
size_t page_start = seg.selected_index / page_size * page_size;
return Bool(ctx->Select(page_start + index));
return (ctx->*verb)(page_start + index);
Copy link
Member

Choose a reason for hiding this comment

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

Used correctly here.

}

Bool RimeSelectCandidate(RimeSessionId session_id, size_t index) {
return do_with_candidate(session_id, index, Select);
Copy link
Member

@lotem lotem Jun 20, 2022

Choose a reason for hiding this comment

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

Prefer explicit conversion to the returned C type (Bool=int):
return Bool(do_with_candidate(...));

Prefer &Select to make it obvious we are passing a function pointer.

}

Bool RimeSelectCandidateOnCurrentPage(RimeSessionId session_id, size_t index) {
return do_with_candidate_on_current_page(session_id, index, Select);
}

const char* RimeGetVersion() {
return RIME_VERSION;
}

Bool RimeDeleteCandidate(RimeSessionId session_id, size_t index) {
return do_with_candidate(session_id, index, DeleteCandidate);
}

Bool RimeDeleteCandidateOnCurrentPage(RimeSessionId session_id, size_t index) {
return do_with_candidate_on_current_page(session_id, index, DeleteCandidate);
}

void RimeSetCaretPos(RimeSessionId session_id, size_t caret_pos) {
an<Session> session(Service::instance().GetSession(session_id));
if (!session)
Expand Down Expand Up @@ -1170,9 +1186,11 @@ RIME_API RimeApi* rime_get_api() {
s_api.get_input = &RimeGetInput;
s_api.get_caret_pos = &RimeGetCaretPos;
s_api.select_candidate = &RimeSelectCandidate;
s_api.delete_candidate = &RimeDeleteCandidate;
Copy link
Member

Choose a reason for hiding this comment

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

Optional: reorder these to match definition in RimeApi.

s_api.get_version = &RimeGetVersion;
s_api.set_caret_pos = &RimeSetCaretPos;
s_api.select_candidate_on_current_page = &RimeSelectCandidateOnCurrentPage;
s_api.delete_candidate_on_current_page = &RimeDeleteCandidateOnCurrentPage;
s_api.candidate_list_begin = &RimeCandidateListBegin;
s_api.candidate_list_next = &RimeCandidateListNext;
s_api.candidate_list_end = &RimeCandidateListEnd;
Expand Down
5 changes: 5 additions & 0 deletions src/rime_api.h
Original file line number Diff line number Diff line change
Expand Up @@ -550,6 +550,11 @@ typedef struct rime_api_t {
void (*commit_proto)(RimeSessionId session_id, RIME_PROTO_BUILDER* commit_builder);
void (*context_proto)(RimeSessionId session_id, RIME_PROTO_BUILDER* context_builder);
void (*status_proto)(RimeSessionId session_id, RIME_PROTO_BUILDER* status_builder);

Copy link
Member

Choose a reason for hiding this comment

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

Merge conflict. Sync the repo.

Copy link
Member

Choose a reason for hiding this comment

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

Sync to latest code.

//! 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.
Bool (*delete_candidate_on_current_page)(RimeSessionId session_id, size_t index);
} RimeApi;

//! API entry
Expand Down