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

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

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

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
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
…809-improve-null-handling-and-initialisation-of-KvkFileWriter
@github-actions github-actions bot added web/ and removed web/ labels Dec 10, 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.

bug(common/web): improve null handling and initialisation of KvkFileWriter
2 participants