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(core): implement loading KMX from blob 🎼 #12459

Merged
merged 8 commits into from
Oct 10, 2024
Merged

feat(core): implement loading KMX from blob 🎼 #12459

merged 8 commits into from
Oct 10, 2024

Conversation

ermshiperete
Copy link
Contributor

@ermshiperete ermshiperete commented Sep 23, 2024 •
edited
Loading

  • split keyboard loading into loading KMX file into blob and then loading the keyboard processor from the blob.
  • deprecate km_core_keyboard_load
  • move file access next to deprecated method. This is now the only place that loads a file in Core; unit tests have some more places that load files.

Part-of: #11293

@keymanapp-test-bot skip

@github-actions github-actions bot added core/ Keyman Core feat labels Sep 23, 2024
@keymanapp-test-bot keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label Sep 23, 2024
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Sep 23, 2024 •
edited
Loading

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

@keymanapp-test-bot keymanapp-test-bot bot changed the title feat(core): implement loading KMX from blob feat(core): implement loading KMX from blob 🎼 Sep 23, 2024
@keymanapp-test-bot keymanapp-test-bot bot added this to the A18S11 milestone Sep 23, 2024
@ermshiperete ermshiperete force-pushed the feat/core/kbd_from_blob branch 6 times, most recently from 95c1b43 to d2494d5 Compare September 23, 2024 16:50
@ermshiperete ermshiperete force-pushed the feat/core/kbd_from_blob branch 8 times, most recently from e7987ac to 2cc932d Compare September 24, 2024 14:27
@ermshiperete
feat(core): implement loading KMX from blob
d06aa29 
- split keyboard loading into loading KMX file into blob and then
  loading the keyboard processor from the blob.
- deprecate `km_core_keyboard_load`
- move file access next to deprecated method. This is now the only place
  that loads a file in Core; unit tests have some more places that
  load files.
- introduce GTest and add unit tests for loading from blob

Part-of: #11293
@ermshiperete ermshiperete force-pushed the feat/core/kbd_from_blob branch from 2cc932d to d06aa29 Compare September 24, 2024 14:30
@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-missing User tests have not yet been defined for the PR label Sep 24, 2024
@ermshiperete ermshiperete marked this pull request as ready for review September 24, 2024 15:25
mcdurdin
mcdurdin requested changes Sep 25, 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.

This is definitely heading in the right direction, getting close to the abstraction we need for web. I'd like to request that we keep the processor factory responsible for determining which processor is required on the basis of the file type. As Mock and Null processors are used only for test purposes, we are free to do what we want with them, and I suggest we use an empty vector for Null processor, and 'MOCK' at start of vector for a Mock processor.

core/include/keyman/keyman_core_api.h Outdated Show resolved Hide resolved
core/src/km_core_keyboard_api.cpp Outdated Show resolved Hide resolved
core/src/km_core_keyboard_api.cpp Outdated Show resolved Hide resolved
core/src/km_core_keyboard_api.cpp Outdated Show resolved Hide resolved
core/src/km_core_keyboard_api.cpp Outdated Show resolved Hide resolved
core/src/km_core_keyboard_api.cpp Outdated Show resolved Hide resolved
core/src/km_core_keyboard_api.cpp Outdated Show resolved Hide resolved
core/src/km_core_keyboard_api.cpp Outdated Show resolved Hide resolved
core/src/km_core_keyboard_api.cpp Outdated Show resolved Hide resolved
core/src/ldml/ldml_processor.cpp Show resolved Hide resolved
@darcywong00 darcywong00 modified the milestones: A18S11, A18S12 Sep 28, 2024
@ermshiperete
Copy link
Contributor Author

ermshiperete commented Oct 1, 2024 •
edited
Loading

This is definitely heading in the right direction, getting close to the abstraction we need for web. I'd like to request that we keep the processor factory responsible for determining which processor is required on the basis of the file type. As Mock and Null processors are used only for test purposes, we are free to do what we want with them, and I suggest we use an empty vector for Null processor, and 'MOCK' at start of vector for a Mock processor.

The question is: do we need this? Is this used anywhere? The only places I could find we directly constructed a Mock processor.

Anyways, done.

This was referenced Oct 2, 2024
Closed
Closed
Closed
Closed
@ermshiperete
feat(core): instantiate different processor types in processor_factory
1c88166 
This moves instantiating the different processor types back to the
`processor_factory` by adding a new `is_handled` static method on the
different processors. Also addresses other code review comments.

Also add some more unit tests.
@ermshiperete ermshiperete requested a review from mcdurdin October 2, 2024 16:47
mcdurdin
mcdurdin approved these changes Oct 7, 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! This is a great refactor. Looking forward to seeing the remaining filesystem access disappear soon 😁

core/src/km_core_keyboard_api.cpp Outdated
} // namespace

km_core_status
km_core_keyboard_load_from_blob_internal(
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't have the km_core_ prefix if it is not a public API.

Suggested change
km_core_keyboard_load_from_blob_internal(
keyboard_load_from_blob_internal(

core/src/kmx/kmx_processor.cpp Outdated Show resolved Hide resolved
@ermshiperete
feat(core): address code review comments
069cd21 
- rename `km_core_keyboard_load_from_blob_internal` to
  `keyboard_load_from_blob_internal` since it's not part of the API
- use size of struct instead of hard-coding minimum KMX file size
@ermshiperete
feat(core): fix build failure on Windows
052ae2e
@ermshiperete
feat(core): fix compilation error on Windows
11a2a3b
@ermshiperete
docs(web): add clarification
db64bad
@ermshiperete ermshiperete requested a review from jahorton as a code owner October 8, 2024 15:39
@github-actions github-actions bot added the web/ label Oct 8, 2024
@ermshiperete ermshiperete force-pushed the feat/core/kbd_from_blob branch 2 times, most recently from 7931a4b to 08d06c3 Compare October 8, 2024 18:43
Base automatically changed from feat/web/readkbd to epic/web-core October 9, 2024 07:02
@ermshiperete ermshiperete force-pushed the feat/core/kbd_from_blob branch from 08d06c3 to 09f89b6 Compare October 9, 2024 07:58
@ermshiperete ermshiperete force-pushed the feat/core/kbd_from_blob branch from 09f89b6 to 2f3edd3 Compare October 9, 2024 08:53
@ermshiperete ermshiperete merged commit fc5b972 into epic/web-core Oct 10, 2024
23 of 24 checks passed
@ermshiperete ermshiperete deleted the feat/core/kbd_from_blob branch October 10, 2024 06:44
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

@rc-swag rc-swag Awaiting requested review from rc-swag rc-swag is a code owner

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

Assignees
No one assigned
Labels
core/ Keyman Core epic-web-core feat linux/ web/
Projects
Keyman
Archived in project
Milestone
A18S12
Development

Successfully merging this pull request may close these issues.
3 participants
@ermshiperete @mcdurdin @darcywong00