Skip to content

refactor(linux): Use km_core_state_context_set_if_needed#10354

Merged
ermshiperete merged 3 commits intomasterfrom
refactor/linux/10212_set-context
Jan 12, 2024
Merged

refactor(linux): Use km_core_state_context_set_if_needed#10354
ermshiperete merged 3 commits intomasterfrom
refactor/linux/10212_set-context

Conversation

@ermshiperete
Copy link
Copy Markdown
Contributor

@ermshiperete ermshiperete commented Jan 10, 2024 •
edited
Loading

Also update comment on km_core_state_context_set_if_needed to make it clearer that it's expecting a UTF-16 string.

Closes #10212.

User Testing

Preparations

Tests

Tests

  • TEST_IPA_WRITER: Open LibreOffice Writer. Switch to "IPA (SIL)" keyboard. Type n>. Verify that the result is "ŋ".
  • TEST_IPA_GEDIT: Open gedit. Switch to "IPA (SIL)" keyboard. Type n>. Verify that the result is "ŋ".
  • TEST_KO_WRITER: Open LibreOffice Writer. Switch to "Korean KORDA Jamo (SIL)" keyboard. Type han<space>geul<space>. Verify that the result is "한글".
  • TEST_KO_GEDIT: Open gedit. Switch to "Korean KORDA Jamo (SIL)" keyboard. Type han<space>geul<space>. Verify that the result is "한글".
  • TEST_KM_WRITER: Open LibreOffice Writer. Switch to "Khmer Angkor" keyboard. Type xEjmr. Verify that the output is "ខ្មែរ".
  • TEST_KM_GEDIT: Open gedit. Switch to "Khmer Angkor" keyboard. Type xEjmr. Verify that the output is "ខ្មែរ".
  • TEST_CONTEXT:
    • Open gedit
    • Switch to "IPA (SIL)" keyboard.
    • Type 
      an
m
    • click after n
    • type >
    • verify that the output is 
      aŋ
m

@keymanapp-test-bot keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label Jan 10, 2024
@keymanapp-test-bot
Copy link
Copy Markdown

keymanapp-test-bot bot commented Jan 10, 2024 •
edited
Loading

User Test Results

Test specification and instructions

  • ✅ TEST_IPA_WRITER (PASSED): 1. Installed the attached PR build keyman 17.0.241-alpha-local (package version 17.0.241-1~PR-10354-1978.1+lunar1) on Ubuntu 23.10 Mantic Minotaur Linux OS (VM) and here is my observation: 1. Opened Libreoffice Writer. 2. Switched to "IPA (SIL)" keyboard. 3. Verified the result was ŋ, after typing "n>". (notes)
  • ✅ TEST_IPA_GEDIT (PASSED): 1. Opened the Text Editor. 2. Switched to "IPA (SIL)" keyboard. Verified the result was ŋ in the notepad. (notes)
  • ✅ TEST_KO_WRITER (PASSED): 1. Opened LibreOffice Writer. 2. Switched to "Korean KORDA Jamo (SIL)" keyboard. 3. Verified the expected result "한글" appeared on the document.
  • ✅ TEST_KO_GEDIT (PASSED): 1. Opened the Text Editor. 2. Switched to "Korean KORDA Jamo (SIL)" keyboard. 3. Verified the expected result "한글" appeared on the text editor.
  • ✅ TEST_KM_WRITER (PASSED): 1. Opened LibreOffice Writer. 2. Switched to "Khmer Angkor" keyboard. 3. Typed xEjmr. Verified the output was ខ្មែរ on the document.
  • ✅ TEST_KM_GEDIT (PASSED): 1. Opened the Text Editor. 2. Switched to "Khmer Angkor" keyboard. 3. Typed xEjmr. Verified the output was ខ្មែរ on the text editor.
  • ✅ TEST_CONTEXT (PASSED): 1. Verified the expected output appeared on the text editor. (notes)

Test Artifacts

@keymanapp-test-bot keymanapp-test-bot bot added this to the A17S30 milestone Jan 10, 2024
@keymanapp-test-bot keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed and removed user-test-missing User tests have not yet been defined for the PR labels Jan 10, 2024
@ermshiperete ermshiperete marked this pull request as ready for review January 10, 2024 17:31
@ermshiperete
refactor(linux): Use km_core_state_context_set_if_needed
9b768c5 
Also update comment on `km_core_state_context_set_if_needed` to make it
clearer that it's expecting a UTF-16 string.

Closes #10212.
@ermshiperete ermshiperete force-pushed the refactor/linux/10212_set-context branch from 4f7ec73 to 9b768c5 Compare January 10, 2024 17:57
mcdurdin
mcdurdin reviewed Jan 11, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

With non-compliant apps, how do we reset the context when the user switches focus?

@ermshiperete
Copy link
Copy Markdown
Contributor Author

ermshiperete commented Jan 11, 2024 •
edited
Loading

With non-compliant apps, how do we reset the context when the user switches focus?

We call km_core_context_clear.

Which raises another question: should we rename this to km_core_state_context_clear (or rather add a new method with that name so that we can deprecate the old one)?

Another option instead of calling km_core_context_clear would be to pass an empty string to km_core_state_context_set_if_needed

@bharanidharanj
Copy link
Copy Markdown

bharanidharanj commented Jan 11, 2024

Test Results

  • TEST_IPA_WRITER (PASSED): 1. Installed the attached PR build keyman 17.0.241-alpha-local (package version 17.0.241-1~PR-10354-1978.1+lunar1) on Ubuntu 23.10 Mantic Minotaur Linux OS (VM) and here is my observation: 1. Opened Libreoffice Writer. 2. Switched to "IPA (SIL)" keyboard. 3. Verified the result was ŋ, after typing "n>".

  • TEST_IPA_GEDIT (PASSED): 1. Opened the Text Editor. 2. Switched to "IPA (SIL)" keyboard. Verified the result was ŋ in the notepad.

  • TEST_KO_WRITER (PASSED): 1. Opened LibreOffice Writer. 2. Switched to "Korean KORDA Jamo (SIL)" keyboard. 3. Verified the expected result "한글" appeared on the document.

  • TEST_KO_GEDIT (PASSED): 1. Opened the Text Editor. 2. Switched to "Korean KORDA Jamo (SIL)" keyboard. 3. Verified the expected result "한글" appeared on the text editor.

  • TEST_KM_WRITER (PASSED): 1. Opened LibreOffice Writer. 2. Switched to "Khmer Angkor" keyboard. 3. Typed xEjmr. Verified the output was ខ្មែរ on the document.

  • TEST_KM_GEDIT (PASSED): 1. Opened the Text Editor. 2. Switched to "Khmer Angkor" keyboard. 3. Typed xEjmr. Verified the output was ខ្មែរ on the text editor.

  • TEST_CONTEXT (PASSED): 1. Verified the expected output appeared on the text editor.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Jan 11, 2024
@mcdurdin
Copy link
Copy Markdown
Member

mcdurdin commented Jan 12, 2024

Which raises another question: should we rename this to km_core_state_context_clear (or rather add a new method with that name so that we can deprecate the old one)?

We can let this wait until 18.0.

Another option instead of calling km_core_context_clear would be to pass an empty string to km_core_state_context_set_if_needed

I'm not 100% sure that will be correct with #10100. I think the explicit clear of context is safer.

mcdurdin
mcdurdin approved these changes Jan 12, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

LGTM

linux/ibus-keyman/src/engine.c
km_core_context_items_dispose(context_items);
g_message("%s: current context is:%zu:%zu:%s:", __FUNCTION__, km_core_context_length(context), buf_size, current_context_utf8);
return current_context_utf8;
static gchar *
Copy link
Copy Markdown
Member

@mcdurdin mcdurdin Jan 12, 2024

Choose a reason for hiding this comment

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

TODO: #10365

rc-swag
rc-swag approved these changes Jan 12, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@rc-swag rc-swag left a comment

Choose a reason for hiding this comment

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

lgtm

@mcdurdin
refactor(core): use km_core_state_context_clear where possible
9830f9e 
Relates to #10365.

Use `km_core_state_context_clear()` instead of `km_core_context_clear()`
as part of eliminating use of `km_core_state_context()`.
@mcdurdin
Copy link
Copy Markdown
Member

mcdurdin commented Jan 12, 2024

Which raises another question: should we rename this to km_core_state_context_clear (or rather add a new method with that name so that we can deprecate the old one)?

We can let this wait until 18.0.

Turns out it already exists! I added it not long ago. See #10370.

@ermshiperete
Merge pull request #10370 from keymanapp/refactor/core/km_core_state_…
16d813d 
…context_clear_1

refactor(core): use `km_core_state_context_clear` where possible
@ermshiperete ermshiperete merged commit 67cf763 into master Jan 12, 2024
@ermshiperete ermshiperete deleted the refactor/linux/10212_set-context branch January 12, 2024 07:54
@keyman-server
Copy link
Copy Markdown
Collaborator

keyman-server commented Jan 12, 2024

Changes in this pull request will be available for download in Keyman version 17.0.243-alpha

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

Reviewers

@mcdurdin mcdurdin mcdurdin approved these changes

@rc-swag rc-swag rc-swag approved these changes

@darcywong00 darcywong00 Awaiting requested review from darcywong00

@sgschantz sgschantz Awaiting requested review from sgschantz

@SabineSIL SabineSIL Awaiting requested review from SabineSIL

Assignees

No one assigned

Labels

core/ Keyman Core developer/ide/ developer/ has-user-test linux/engine/ linux/ mac/ refactor

Projects

None yet

Milestone

A17S30

Development

Successfully merging this pull request may close these issues.

refactor(linux): ibus: use km_core_state_context_set_if_needed

5 participants

@ermshiperete @bharanidharanj @mcdurdin @keyman-server @rc-swag