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.

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

feat(web): load .kmx keyboard from blob 🎼 #12823

Open
wants to merge 1 commit into
base: fix/core/alignment
Choose a base branch
from

Conversation

ermshiperete
Copy link
Contributor

This change adds the web side of loading a .kmx keyboard from a blob. It also replaces the CoreProcessor class with CoreFactory that allows to directly use the methods defined in WASM without having to add another wrapper for each method.

Note that this is only the first step that implements loading the kmx keyboard, but we don't anything with it yet.

Part-of: #11293
@keymanapp-test-bot skip

@github-actions github-actions bot added the web/ label Dec 11, 2024
@keymanapp-test-bot keymanapp-test-bot bot changed the title feat(web): load .kmx keyboard from blob feat(web): load .kmx keyboard from blob 🎼 Dec 11, 2024
@github-actions github-actions bot added core/ Keyman Core feat labels Dec 11, 2024
@keymanapp-test-bot keymanapp-test-bot bot added this to the A18S17 milestone Dec 11, 2024
@ermshiperete ermshiperete marked this pull request as draft December 11, 2024 18:49
}
});
} catch (e: any) {
console.log('got execption in CoreFactory.createCoreProcessor', e);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
console.log('got execption in CoreFactory.createCoreProcessor', e);
console.log('got exception in CoreFactory.createCoreProcessor', e);

Copy link
Member

Choose a reason for hiding this comment

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

I think we need to ensure that exceptions here aren't masked. Console log is not available in many contexts e.g. System keyboard. Console.error is a start but not enough maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you suggest?

Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason the exception needs to be masked here? Could it just change to propagate?

Base automatically changed from feat/core/exportapiwasmcore to epic/web-core December 17, 2024 07:30
This change adds the web side of loading a .kmx keyboard from a
blob. It also replaces the `CoreProcessor` class with `CoreFactory`
that allows to directly use the methods defined in WASM without
having to add another wrapper for each method.
@ermshiperete ermshiperete changed the base branch from epic/web-core to fix/core/alignment December 20, 2024 19:24
@ermshiperete ermshiperete marked this pull request as ready for review December 20, 2024 19:24
non-zero value is an error).

## Specification

-->
// keep in sync with web/src/engine/core-processor/src/core-factory.ts
Copy link
Member

Choose a reason for hiding this comment

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

}
});
} catch (e: any) {
console.log('got execption in CoreFactory.createCoreProcessor', e);
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason the exception needs to be masked here? Could it just change to propagate?

it('can initialize without errors', async function () {
const kp = new CoreProcessor();
assert.isTrue(await kp.init(coreurl));
assert.isNotNull(await CoreFactory.createCoreProcessor(coreurl));
Copy link
Member

Choose a reason for hiding this comment

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

here for example would fail if there's an err.

@darcywong00 darcywong00 modified the milestones: A18S17, A18S18 Dec 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants