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(ios): cross-paragraph keyboard rules #10905

Merged
merged 1 commit into from
Mar 4, 2024

Conversation

jahorton
Copy link
Contributor

@jahorton jahorton commented Mar 1, 2024

Fixes #10761.

Turns out my concerns in that issue were well-founded; rules crossing a context-window boundary were not being properly applied. Fortunately, it only takes a couple of small tweaks to fix that... as well as can be done under iOS constraints given 17.0.

Limitations:

  • If a surrogate pair exists out-of-context that would be deleted by such a rule, an extra character will be deleted.
  • If a combining character exists out-of-context that would be deleted by such a rule, an extra character will be deleted.

To "do better" would require us to track our own version of a context window manually on the iOS side that slides, rather than "jumping" upon a new paragraph start. This is only an idea at this stage, though; it's also certainly too big a project to tackle during the beta cycle.

Given these limitations, I'm not opposed to rejecting this PR at this time due to what is essentially an incomplete solution - those caveats may matter greatly for some scripts and/or languages.

User Testing

TEST_CROSS_PARAGRAPH_RULES: Use the provided specialized test keyboard to verify that the last a-z character is properly rotated by the "rota" key.

Specialized test keyboard: https://jahorton.github.io/last_alphanumeric_rotater.kmp
Source: last_alphanumeric_rotater.zip

  • Should this be added to common/test/keyboards? (For desktop use, the 'rota' key is K_BKSLASH.)

frame 2

  1. Install the test keyboard package.
  2. With it installed, type 'a', then hit the ENTER key twice.
  3. Tap the "rota" key repeatedly. The original a should rotate to b, then c, etc until reaching z. After z, it will return to a.
  4. If any other effects occur, FAIL this test.

@jahorton jahorton requested a review from sgschantz as a code owner March 1, 2024 03:39
@keymanapp-test-bot keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed labels Mar 1, 2024
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Mar 1, 2024

User Test Results

Test specification and instructions

  • TEST_CROSS_PARAGRAPH_RULES (PASSED): Tested with the attached PR build 17.0.280-beta-test-10905 on an iPhone 13 Mobile device and here is my observation: 1. Installed last_alpharnumeric_rotater keyboard. 2. Typed 'a', then hit the ENTER key twice. 3. Clicked the "rota" key repeatedly and verified that it reached the z key then returned to show a key. Seems to be working as expected.

Test Artifacts

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 think this change makes sense for robustness, even if we want to go further in the future.

LGTM

@bharanidharanj
Copy link

Test Results

  • TEST_CROSS_PARAGRAPH_RULES (PASSED): Tested with the attached PR build 17.0.280-beta-test-10905 on an iPhone 13 Mobile device and here is my observation: 1. Installed last_alpharnumeric_rotater keyboard. 2. Typed 'a', then hit the ENTER key twice. 3. Clicked the "rota" key repeatedly and verified that it reached the z key then returned to show a key. Seems to be working as expected.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Mar 1, 2024
@mcdurdin mcdurdin modified the milestones: B17S2, B17S3 Mar 3, 2024
@jahorton jahorton merged commit 691fce2 into beta Mar 4, 2024
6 checks passed
@jahorton jahorton deleted the fix/ios/cross-paragraph-kbd-rules branch March 4, 2024 03:04
@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 17.0.281-beta

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants