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

feat(linux): use new km_core_keyboard_load_from_blob API method 🎼 #12683

Merged
merged 3 commits into from
Nov 25, 2024
Merged

feat(linux): use new km_core_keyboard_load_from_blob API method 🎼 #12683

merged 3 commits into from
Nov 25, 2024

Conversation

ermshiperete
Copy link
Contributor

@ermshiperete ermshiperete commented Nov 22, 2024 •
edited
Loading

Fixes: #12498

User Testing

TEST_WORKS: Keyman keyboards should still work as before, i.e. switch to a Keyman keyboard and verify that it behaves as expected.

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

keymanapp-test-bot bot commented Nov 22, 2024 •
edited
Loading

User Test Results

Test specification and instructions

Test Artifacts

@keymanapp-test-bot keymanapp-test-bot bot changed the title feat(linux): use new km_core_keyboard_load_from_blob API method feat(linux): use new km_core_keyboard_load_from_blob API method 🎼 Nov 22, 2024
@keymanapp-test-bot keymanapp-test-bot bot added this to the A18S15 milestone Nov 22, 2024
mcdurdin
mcdurdin approved these changes Nov 22, 2024
View reviewed changes
Copy link
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 Outdated
Comment on lines 525 to 527
fseek(kmx_file, 0, SEEK_END);
long length = ftell(kmx_file);
fseek(kmx_file, 0, SEEK_SET);
Copy link
Member

Choose a reason for hiding this comment

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

No error checking?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok ok. Done 😄

@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 Nov 23, 2024
@darcywong00 darcywong00 modified the milestones: A18S15, A18S16 Nov 24, 2024
@dinakaranr
Copy link

Test Results

I tested this issue with the attached keyman 18.0.138-alpha (package version
18.0.138-1~PR-12683-4025.1+focal1) build on the Jammy & Focal OS environment. Here is my observation.

  • TEST_WORKS (Passed):
  1. Installed the Keyman package version 118.0.138-1~PR-12683-4025.1+focal1
  2. Open the configuration dialog by running the "km-config" command in the terminal.
  3. "Download Keyman keyboards" dialog by clicking the "Download keyboard" button.
  4. Installed the SIL IPA keyboard.(search a keyboard in the search box)(Khmer, gff_Amheric)
  5. Verified that the installed keyboard appears on the configuration dialog.
  6. Open the Note editor & Libre Office application
  7. Switch the keyboard from English to Khmer.
  8. Enter some words in the application.
  9. Switch the keyboard from Khmer to IPA.
  10. Enter some words in the application.
  11. Verified the text appeared in the application and the keyboard switch works well.
    It works well with Jammy and Focal OSs. Thank you.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Nov 25, 2024
@ermshiperete ermshiperete merged commit 63b83bd into epic/web-core Nov 25, 2024
8 checks passed
@ermshiperete ermshiperete deleted the feat/linux/12498_blob branch November 25, 2024 09:56
@ermshiperete ermshiperete mentioned this pull request Nov 26, 2024
Closed
@mcdurdin mcdurdin linked an issue Nov 27, 2024 that may be closed by this pull request
refactor(linux): implement loading of keyboard blob in Keyman for Linux #12498
Closed
ermshiperete added a commit that referenced this pull request Nov 27, 2024
@ermshiperete
feat(linux): use new km_core_keyboard_load_from_blob API method
8492b12 
Fixes: #12498
Cherry-pick: #12683
@ermshiperete ermshiperete mentioned this pull request Nov 27, 2024
Open
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

@darcywong00 darcywong00 Awaiting requested review from darcywong00 darcywong00 is a code owner

Assignees
No one assigned
Labels
epic-web-core feat has-user-test linux/engine/ linux/
Projects
Keyman
Status: Done
Milestone
A18S16
Development

Successfully merging this pull request may close these issues.

refactor(linux): implement loading of keyboard blob in Keyman for Linux
4 participants
@ermshiperete @dinakaranr @mcdurdin @darcywong00