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.

Sign up for GitHub

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

Jump to bottom

Add new APIs, peek selection and change page #620

Closed
wants to merge 5 commits into from
Closed

Add new APIs, peek selection and change page #620

wants to merge 5 commits into from

Conversation

LEOYoon-Tsaw
Copy link
Member

@LEOYoon-Tsaw LEOYoon-Tsaw commented Feb 20, 2023 •
edited
Loading

Context:Peek

Pull request

Issue tracker

Partially addresses #616

Feature

Add 3 new APIs:

  1. peek_candidate (similar to select_candidate, but without committing to the selection)
  2. peek_candidate_on_current_page
  3. change_page (page up and down in the same function, specified by parameter)

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

@LEOYoon-Tsaw
Copy link
Member Author

macOS build fails due to this innovation, not my fault

clang: error: invalid version number in 'MACOSX_DEPLOYMENT_TARGET=$(RECOMMENDED_MACOSX_DEPLOYMENT_TARGET)'

@LEOYoon-Tsaw LEOYoon-Tsaw mentioned this pull request Apr 14, 2023
Merged
@mokapsing mokapsing mentioned this pull request May 25, 2023
Closed
2 tasks
ksqsf
ksqsf reviewed Feb 3, 2024
View reviewed changes
Copy link
Member

@ksqsf ksqsf left a comment •
edited
Loading

Choose a reason for hiding this comment

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

目前鼠须管master分支依赖该PR，导致build不便，鼠须管主线也无法直接更新到librime 1.9，因此非常希望该PR尽早合并，或至少把依赖该PR的鼠须管代码移除、另作PR。

@lotem comments?

src/rime_api.h Outdated
@@ -524,6 +524,8 @@ typedef struct rime_api_t {

//! select a candidate at the given index in candidate list.
Bool (*select_candidate)(RimeSessionId session_id, size_t index);
//! peek a selection without commiting to it
Bool (*peek_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.

把API放在struct中间会破坏ABI兼容性，新API应置于struct末尾。下同

@lotem
Copy link
Member

lotem commented Feb 8, 2024 •
edited
Loading

目前鼠须管master分支依赖该PR，导致build不便，鼠须管主线也无法直接更新到librime 1.9，因此非常希望该PR尽早合并，或至少把依赖该PR的鼠须管代码移除、另作PR。

@lotem comments?

我先嘗試把 squirrel 編譯過，現在第一步在 Sparkle 那裏出錯了。——給submodule升級解決

@lotem
Copy link
Member

lotem commented Feb 8, 2024 •
edited
Loading

想就代碼變更討論兩點：

  1. Peek 按他的原意是偷看，librime 代碼裏也用到，表示提前看下一個候選詞但不從 Translation 或 Menu 裏取走。這是個無副作用的操作。

這個 PR 與其類似，但同時還改變了 Segment::selected_index ，這下就是改變了 Context 的內容和用戶看到的結果：選中的候選詞發生變動。

我建議換用一個表示出發生這種變動的名字，如 SetSelectedIndex、SetSelectedCandidate。或者選一個簡練概括的詞如 Hightlight。

  1. Peek 的返回值，要不要在選中的詞沒有變化時返回 false。對應兩種情況：給的 index 超出候選詞序號的範圍；index 與 Segment::selected_index 的值相同。如果當前選中的是最後一頁的最後一個候選字，這時嘗試選中下一頁的候選序數，如果按照建議的修改，會得到 false 的結果，前端可以據此決定無需更新界面等等。

@LEOYoon-Tsaw @ksqsf

lotem
lotem requested changes Feb 8, 2024
View reviewed changes
Copy link
Member

@lotem lotem left a comment

Choose a reason for hiding this comment

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

I've got some time working on this PR, and I put my comments down for what changes I think are necessary.

src/rime/context.cc
select_notifier_(this);
return true;
size_t new_index = index;
if (index < 0) {
Copy link
Member

Choose a reason for hiding this comment

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

index < 0 does not happen with size_t.

src/rime/context.cc
size_t candidate_count = seg.menu->Prepare(index + 1);
if (index >= candidate_count) {
DLOG(INFO) << "selection index exceed candidate pool, fallback to last";
new_index = candidate_count - 1;
Copy link
Member

Choose a reason for hiding this comment

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

The fallback feature is convenient in highlighting the candidate of the same label number in the next page.

But when called by Select(index), the given index means a definite candidate, and fallback is unexpected.

src/rime/context.cc
@@ -110,17 +110,35 @@ void Context::Clear() {
}

bool Context::Select(size_t index) {
if (composition_.empty())
bool result = Peek(index);
Copy link
Member

Choose a reason for hiding this comment

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

The fallback mechanism of Peek does not work for Select. See my other comment below.

Also, this creates a difference in the signaled notifications: an extra update_notifier_ is triggered before select_notifier_.

I think we'd better keep the two functions separate.

@lotem
Copy link
Member

lotem commented Feb 8, 2024

我正在改，再改改。

@lotem
Copy link
Member

lotem commented Feb 9, 2024

沒意見我就我按自己的愛好改咯。

lotem pushed a commit to lotem/librime that referenced this pull request Feb 9, 2024
@LEOYoon-Tsaw @lotem
feat(api): highlight_candidate*, change_page
142902d 
add function Context::Highlight

Merges rime#620

Squashed commit of the following:

commit bff5a89a8abb31c0ca612e42fe704cb8b522e2df
Author: 居戎氏 <[email protected]>
Date:   Fri Feb 9 11:01:36 2024 +0800

    rename the new function Context::Highlight

    do not re-use it in Context::Select

commit fbc709e
Author: 居戎氏 <[email protected]>
Date:   Thu Feb 8 23:03:43 2024 +0800

    re-order API functions

commit 0de4da0
Author: 居戎氏 <[email protected]>
Date:   Thu Feb 8 22:20:35 2024 +0800

    fix: missing brackets in if-statement

commit 5c1502e
Author: LEO Yoon-Tsaw <[email protected]>
Date:   Tue Feb 21 09:49:34 2023 -0500

    Fix RECOMMENDED_MACOSX_DEPLOYMENT_TARGET error

commit ecd3649
Author: LEO Yoon-Tsaw <[email protected]>
Date:   Mon Feb 20 16:42:45 2023 -0500

    Update context.cc

commit 9aa83e6
Author: LEO Yoon-Tsaw <[email protected]>
Date:   Sun Feb 19 23:56:17 2023 -0500

    Add new APIs, peek selection and change page

    Context:Peek
@lotem
Copy link
Member

lotem commented Feb 9, 2024

Modified and merged as 142902d

@lotem lotem closed this Feb 9, 2024
graphemecluster pushed a commit to TypeDuck-HK/librime that referenced this pull request Mar 18, 2024
@LEOYoon-Tsaw @graphemecluster
feat(api): highlight_candidate*, change_page
3a3330e 
add function Context::Highlight

Merges rime#620

Squashed commit of the following:

commit bff5a89a8abb31c0ca612e42fe704cb8b522e2df
Author: 居戎氏 <[email protected]>
Date:   Fri Feb 9 11:01:36 2024 +0800

    rename the new function Context::Highlight

    do not re-use it in Context::Select

commit fbc709e
Author: 居戎氏 <[email protected]>
Date:   Thu Feb 8 23:03:43 2024 +0800

    re-order API functions

commit 0de4da0
Author: 居戎氏 <[email protected]>
Date:   Thu Feb 8 22:20:35 2024 +0800

    fix: missing brackets in if-statement

commit 5c1502e
Author: LEO Yoon-Tsaw <[email protected]>
Date:   Tue Feb 21 09:49:34 2023 -0500

    Fix RECOMMENDED_MACOSX_DEPLOYMENT_TARGET error

commit ecd3649
Author: LEO Yoon-Tsaw <[email protected]>
Date:   Mon Feb 20 16:42:45 2023 -0500

    Update context.cc

commit 9aa83e6
Author: LEO Yoon-Tsaw <[email protected]>
Date:   Sun Feb 19 23:56:17 2023 -0500

    Add new APIs, peek selection and change page

    Context:Peek
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ksqsf ksqsf ksqsf left review comments

@lotem lotem lotem requested changes

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 @lotem @ksqsf