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

chore(core): remove km_core_keyboard_load API #12769

Merged
merged 3 commits into from
Dec 5, 2024
Merged

chore(core): remove km_core_keyboard_load API #12769

merged 3 commits into from
Dec 5, 2024

Conversation

mcdurdin
Copy link
Member

@mcdurdin mcdurdin commented Dec 4, 2024

Fixes: #12497

User Testing

TEST_WINDOWS: With Keyman for Windows, Test any Keyman keyboard, that it loads successfully and can be used in apps.
TEST_MAC: With Keyman for macOS, Test any Keyman keyboard, that it loads successfully and can be used in apps.
TEST_LINUX: With Keyman for Linux, Test any Keyman keyboard, that it loads successfully and can be used in apps.
TEST_DEVELOPER: Load a keyboard .kmn file into the debugger, and verify that the keyboard can be used in the keyboard debugger.

@keymanapp-test-bot keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed labels Dec 4, 2024
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Dec 4, 2024 •
edited
Loading

User Test Results

Test specification and instructions

Test Artifacts

@keymanapp-test-bot keymanapp-test-bot bot added this to the A18S16 milestone Dec 4, 2024
ermshiperete
ermshiperete requested changes Dec 4, 2024
View reviewed changes
Copy link
Contributor

@ermshiperete ermshiperete left a comment

Choose a reason for hiding this comment

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

We'll have to let the FCITX guys know that they will have to change their code for v18

core/docs/api/keyboards.md Outdated Show resolved Hide resolved
core/src/km_core_keyboard_api.cpp
@@ -40,16 +40,18 @@ namespace
} // namespace

km_core_status
keyboard_load_from_blob_internal(
km_core_keyboard_load_from_blob(
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, removing keyboard_load_from_blob_internal means that we'll have to initialize a std::vector twice when called from WASM. Don't know how much overhead that has.

Copy link
Member

Choose a reason for hiding this comment

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

well— could retain keyboard_load_from_blob_internal passing in the vector?

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently, we don't have the load from blob code in epic/web-core, is that right? Happy for a refactor back if that makes sense, but as it isn't landing in 18.0, I thought we could cleanup the source here at this point, given the km_core_keyboard_load_from_blob was literally a wrap-in-vector and call internal two liner.

core/tests/unit/km_core_keyboard_api.tests.cpp Outdated Show resolved Hide resolved
@ermshiperete ermshiperete mentioned this pull request Dec 4, 2024
Open
@ermshiperete
Copy link
Contributor

We'll have to let the FCITX guys know that they will have to change their code for v18

fcitx/fcitx5-keyman#7

@dinakaranr
Copy link

Test Results

I tested this issue with the attached Keyman 18.0.152-alpha-local build in the Windows, Ubuntu, KeymanDeveloper, and macOS environments. Here is my observation.

  • TEST_WINDOWS (Passed):
  1. Installed the Keyman package version keyman-18.0.152-alpha-test-12769.exe.
  2. Open the configuration dialog.
  3. "Download Keyman keyboards" dialog by clicking the "Download keyboard" button.
  4. Installed the "Khmer Angkor" keyboard.(search a keyboard in the search box)(Khmer, gff_Amheric, SIL_IPA, Hindi, EuroLatin)
  5. Verified that the installed keyboard appears on the configuration dialog.
  6. Open the Notepad editor & Libra Office application.
  7. Switch the keyboard from English to Khmer.
  8. Enter some words in the application.
  9. Verified that the "Khmer" letters appeared
  10. Open the Google search box.
  11. Switch the keyboard from Khmer to IPA.
  12. Enter some words in the application.
  13. Verified that the "IPA" letters appeared

It works well.

  • TEST_MAC (Passed):
  1. Installed the Keyman package version keyman-18.0.152.dmg
  2. Open the configuration dialog.
  3. "Download Keyman keyboards" dialog by clicking the "Download keyboard" button.
  4. Installed the "Khmer Angkor" keyboard.(search a keyboard in the search box)(Khmer, gff_Amheric, Hindi, EuroLatin)
  5. Verified that the installed keyboard appears on the configuration dialog.
  6. Open the Note editor & Libra Office application.
  7. Switch the keyboard from English to Khmer.
  8. Enter some words in the application.
  9. Verified that the "Khmer" letters appeared
  10. Open the Stickies & Note application.
  11. Switch the keyboard from Khmer to IPA.
  12. Enter some words in the application.
  13. Verified that the "IPA" letters appeared

It works well.

  • TEST_LINUX (Passed):
  1. Installed the Keyman package version keyman-18.0.152-1~PR-12769-4096.1+jammy1
  2. Open the configuration dialog.
  3. "Download Keyman keyboards" dialog by clicking the "Download keyboard" button.
  4. Installed the "Khmer Angkor" keyboard.(search a keyboard in the search box)(Khmer, gff_Amheric, SIL_IPA, Hindi, EuroLatin)
  5. Verified that the installed keyboard appears on the configuration dialog.
  6. Open the g editor & Libra Office application.
  7. Switch the keyboard from English to Khmer.
  8. Enter some words in the application.
  9. Verified that the "Khmer" letters appeared
    It works well.
  • TEST_DEVELOPER (Passed):
  1. Installed the Keyman package version keyman-18.0.152-alpha-test-12769.exe
  2. On Keyman Developer, Open the Khmer keyboard project.
  3. Navigate to the "Keyboard" tab and then click the *.kmn link button to navigate to the build tab.
  4. Click the "Compile Keyboard" button.
  5. Click the "Start Debugging" button or Press the F5 key.
  6. Debugger mode was opened
  7. Type a sentence in an empty box.
  8. Verified that the "Khmer" text appeared and the text grid showed the text too.
    It works well. Thank you.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Dec 4, 2024
srl295
srl295 reviewed Dec 4, 2024
View reviewed changes
Copy link
Member

@srl295 srl295 left a comment

Choose a reason for hiding this comment

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

Looks good once @ermshiperete comments are accepted!

core/docs/api/keyboards.md Outdated Show resolved Hide resolved
core/src/km_core_keyboard_api.cpp
@@ -40,16 +40,18 @@ namespace
} // namespace

km_core_status
keyboard_load_from_blob_internal(
km_core_keyboard_load_from_blob(
Copy link
Member

Choose a reason for hiding this comment

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

well— could retain keyboard_load_from_blob_internal passing in the vector?

@mcdurdin
Copy link
Member Author

mcdurdin commented Dec 4, 2024

  • TEST_DEVELOPER (Passed):
  1. Installed the Keyman package version keyman-18.0.152-alpha-test-12769.exe

@dinakaranr, did you install keyman or keyman developer for this test?

@mcdurdin
Copy link
Member Author

mcdurdin commented Dec 5, 2024

@ermshiperete ready for re-review

@mcdurdin mcdurdin merged commit 88f20d7 into master Dec 5, 2024
20 checks passed
@mcdurdin mcdurdin deleted the chore/core/12497-remove-km_core_keyboard_load branch December 5, 2024 22:51
@ermshiperete ermshiperete mentioned this pull request Dec 6, 2024
Merged
@keyman-server
Copy link
Collaborator

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

rc-swag
rc-swag reviewed Dec 10, 2024
View reviewed changes
Copy link
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
even though already mereged :)

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

@rc-swag rc-swag rc-swag left review comments

@srl295 srl295 srl295 approved these changes

@ermshiperete ermshiperete ermshiperete approved these changes

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

Assignees
No one assigned
Labels
chore core/ Keyman Core has-user-test linux/ windows/engine/ windows/
Projects
Keyman
Archived in project
Milestone
A18S16
Development

Successfully merging this pull request may close these issues.

chore(core): Deprecate km_core_keyboard_load
6 participants
@mcdurdin @ermshiperete @dinakaranr @keyman-server @srl295 @rc-swag