feat: delete selected candidate #556

InfinityLoop1308 · 2022-06-18T07:53:46Z

Pull request

Issue tracker

Fixes will automatically close the related issue

Fixes #

Feature

This feature supports deleting candidate at arbitrary index, which is useful for quick deleting.

Unit test

Done

Manual test

Done

Code Review

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

Additional Info

lotem

Thanks for the pull request.

The struct RimeApi can only be extended from the end for backward compatibility.
Sync the repo, and then add new API functions as the last members of the struct.

I also added a few suggestions about code style.

lotem · 2022-06-19T16:32:51Z

src/rime_api.cc

@@ -1083,6 +1083,34 @@ const char* RimeGetVersion() {
  return RIME_VERSION;
 }

+Bool RimeDeleteCandidate(RimeSessionId session_id, size_t index) {
+  an<Session> session(Service::instance().GetSession(session_id));


avoid repeated code.

extract the common code to helper functions:

static bool do_with_candidate(RimeSessionId session_id, size_t index, bool (Context::* verb)(size_t index)) { // ... return (ctx->*verb)(index); } static bool do_with_candidate_on_current_page(RimeSessionId session_id, size_t index, bool (Context::* verb)(size_t index)) { // ... return (ctx->*verb)(page_start + index); }

Then call the helper functions with &Context::Select or &Context::Delete in each version.

Sorry but it turns out I have trouble using this syntax. It first throw error message with
/home/runner/work/trime/trime/app/src/main/jni/librime/src/rime_api.cc:1061:25: error: called object type 'bool

(rime::Context::*)(size_t)' is not a function or function pointer

return Bool(ctx->*verb(index)); ~~~~^

/home/runner/work/trime/trime/app/src/main/jni/librime/src/rime_api.cc:1083:53: error: use of undeclared identifier 'Select'; did you mean 'rime::Context::Select'?

return Bool(do_with_candidate(session_id, index, &Select)); ^~~~~~ rime::Context::Select

/home/runner/work/trime/trime/app/src/main/jni/librime/src/rime/context.h:47:8: note: 'rime::Context::Select' declared here

bool Select(size_t index); ^

and then after I change &Select to &rime::Context::Select it throw error message:

home/runner/work/trime/trime/app/src/main/jni/librime/src/rime_api.cc:1061:25: error: called object type 'bool (rime::Context::*)(size_t)' is not a function or function pointer

return Bool(ctx->*verb(index)); ~~~~^

1 error generated.

pointer to member function, if I recall correctly, need to be qualified by class name, try &Context::Select as in the original comment.

pointer to member function, if I recall correctly, need to be qualified by class name, try &Context::Select as in the original comment.

it not works either.
I have changed to

Bool RimeSelectCandidate(RimeSessionId session_id, size_t index) {
return Bool(do_with_candidate(session_id, index, &Context::Select));
}

and got error message:

home/runner/work/trime/trime/app/src/main/jni/librime/src/rime_api.cc:1061:25: error: called object type 'bool (rime::Context::*)(size_t)' is not a function or function pointer
return Bool(ctx->*verb(index));

Sorry. It didn't work because .* or ->* has lower precedence than function call. So it should be written with parentheses around the expression ctx->*verb. See also my inline comments.

lotem · 2022-06-19T16:33:28Z

src/rime_api.h

@@ -520,6 +520,9 @@ typedef struct rime_api_t {
  //! select a candidate at the given index in candidate list.
  Bool (*select_candidate)(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);


New API functions must be appended to the struct RimeApi.

lotem · 2022-06-19T16:53:30Z

src/rime/context.cc

@@ -123,6 +123,19 @@ bool Context::Select(size_t index) {
  return false;
 }

+bool Context::Delete(size_t index) {


The name is unclear; it can be confused with deleting characters from the input.
Rename it DeleteCandidate.

Avoid repeated code.
Introduce a private member function (overloaded)

bool Context::DeleteCandidate(function<an<Candidate> (Segment& seg)> get_candidate) { // ... if (auto cand = get_candidate(seg)) { // ... }

Then call it in each version:

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(); }); }

lotem

Thanks, PTAL

lotem · 2022-06-20T00:54:09Z

src/rime_api.cc

@@ -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;


Optional: reorder these to match definition in RimeApi.

lotem · 2022-06-20T00:54:31Z

src/rime_api.h

@@ -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);
+


Merge conflict. Sync the repo.

Sync to latest code.

lotem · 2022-06-20T00:56:08Z

src/rime/context.cc

@@ -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) {


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

lotem · 2022-06-20T01:01:41Z

src/rime_api.cc

+}
+
+Bool RimeSelectCandidate(RimeSessionId session_id, size_t index) {
+  return do_with_candidate(session_id, index, Select);


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.

lotem · 2022-06-22T16:42:55Z

src/rime_api.cc

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


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

lotem · 2022-06-22T16:43:30Z

src/rime_api.cc

@@ -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);


Used correctly here.

lotem · 2022-06-22T16:43:54Z

src/rime_api.h

@@ -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);
+


Sync to latest code.

lotem · 2022-06-22T16:47:48Z

src/rime_api.cc

@@ -1083,6 +1083,34 @@ const char* RimeGetVersion() {
  return RIME_VERSION;
 }

+Bool RimeDeleteCandidate(RimeSessionId session_id, size_t index) {
+  an<Session> session(Service::instance().GetSession(session_id));


Sorry. It didn't work because .* or ->* has lower precedence than function call. So it should be written with parentheses around the expression ctx->*verb. See also my inline comments.

InfinityLoop1308 · 2022-06-23T10:01:58Z

Ok so it turns out lacking an build environment made a mess of everything. After I managed to fix the environment, the IDE immediately pointed the mistake and fix it, and everything is working well now. This PR will be closed, and a new PR will be open to make commit tree clean.

Thanks for your patience these days.

lotem requested changes Jun 19, 2022

View reviewed changes

change

ad3e7f9

lotem reviewed Jun 20, 2022

View reviewed changes

lotem requested changes Jun 22, 2022

View reviewed changes

InfinityLoop1308 closed this Jun 23, 2022

InfinityLoop1308 deleted the feat_delete_selected_candidate branch June 23, 2022 10:02

InfinityLoop1308 mentioned this pull request Jun 23, 2022

feat: delete selected candidate #557

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: delete selected candidate #556

feat: delete selected candidate #556

InfinityLoop1308 commented Jun 18, 2022 •

edited

Loading

lotem left a comment

lotem Jun 19, 2022

InfinityLoop1308 Jun 20, 2022

lotem Jun 20, 2022

InfinityLoop1308 Jun 21, 2022

lotem Jun 22, 2022

lotem Jun 19, 2022

lotem Jun 19, 2022

lotem left a comment

lotem Jun 20, 2022

lotem Jun 20, 2022

lotem Jun 22, 2022

lotem Jun 20, 2022

lotem Jun 20, 2022 •

edited

Loading

lotem Jun 22, 2022

lotem Jun 22, 2022

lotem Jun 22, 2022

lotem Jun 22, 2022

InfinityLoop1308 commented Jun 23, 2022

feat: delete selected candidate #556

feat: delete selected candidate #556

Conversation

InfinityLoop1308 commented Jun 18, 2022 • edited Loading

Pull request

Issue tracker

Feature

Unit test

Manual test

Code Review

Additional Info

lotem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lotem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lotem Jun 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

InfinityLoop1308 commented Jun 23, 2022

InfinityLoop1308 commented Jun 18, 2022 •

edited

Loading

lotem Jun 20, 2022 •

edited

Loading