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

fix(common/web): improve null handling and initialisation of KvkFileWriter #12810

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

fix(common/web): improve null handling and initialisation of KvkFileWriter #12810

wants to merge 3 commits into from

Conversation

markcsinclair
Copy link
Contributor

Improve null handling of associatedKeyboard, key ansiFont.name and unicodeFont.name in KvkFileWriter, plus incorrect initialisation of BUILDER_KVK_STRING, which should be { len: 1, str: '' } not { len: 0, str: '' }.

Test cases included to cover the three null strings.

Fixes: #12809

@keymanapp-test-bot skip

@keymanapp-test-bot
Copy link

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

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

@keymanapp-test-bot keymanapp-test-bot bot added this to the A18S17 milestone Dec 9, 2024
@markcsinclair markcsinclair self-assigned this Dec 9, 2024
@markcsinclair markcsinclair marked this pull request as draft December 9, 2024 11:10
@markcsinclair markcsinclair changed the base branch from master to test/common/web/types/9052-unit-tests-kvk-file-writer December 9, 2024 11:12
mcdurdin
mcdurdin reviewed Dec 9, 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.

I'd like to verify this behaviour against the Delphi .kvk file writer -- I am not 100% sure it is correct. I won't have time to look at this tonight unfortunately.

https://github.com/keymanapp/keyman/blob/1eff4dbca48d0d81b2ee70322e314c56911b4368/common/windows/delphi/visualkeyboard/VisualKeyboardSaverBinary.pas

Base automatically changed from test/common/web/types/9052-unit-tests-kvk-file-writer to master December 10, 2024 10:47
@markcsinclair
chore(common/web): Merge branch 'master' into fix/common/web/types/12…
dabe1b9 
…809-improve-null-handling-and-initialisation-of-KvkFileWriter
@github-actions github-actions bot added web/ and removed web/ labels Dec 10, 2024
@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
Reviewers

@mcdurdin mcdurdin mcdurdin left review comments

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

At least 1 approving review is required to merge this pull request.

Assignees

@markcsinclair markcsinclair

Labels
common/web/ common/ fix web/
Projects
Keyman
Status: No status
Milestone
A18S18
Development

Successfully merging this pull request may close these issues.

bug(common/web): improve null handling and initialisation of KvkFileWriter
3 participants
@markcsinclair @mcdurdin @darcywong00