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): generalized multitap 🐵 #9740

Merged
merged 21 commits into from
Nov 13, 2023

Conversation

jahorton
Copy link
Contributor

@jahorton jahorton commented Oct 11, 2023

Now for something exciting - implementation of one of our long-anticipated new gesture types... the multitap. This is one of the big goals of #5029.

multitap-hello-world-2

That said... wow, the complications that arise when a key can result in both a modipress and multitap! Resolving that took significant effort and care... and admittedly, modipress still isn't actually implemented. The new 'modipress-multitap-* gesture-model specs are necessary in order to allow a modipressable key to multitap; modipresses should layer-swap on keydown and fully resolve on keydown+time - which requires two models, while multitaps are triggered by the keyup after the keydown that instantly meets the conditions necessary for modipress's layer-swap event.

Short version... I implemented "enough" of modipress to ensure that multitaps can work on modipress keys, but went no further. Modipress state management hasn't been implemented yet.

Not yet done as of this PR:

Currently done:

  • text output properly rewinds to the state before the base key before evaluating keyboard rules
  • next layer management - even on multitaps with text output
  • 'rota'-style behavior if continuously tapped
  • configuration possible for multitap timers: for max length of wait between keys and max length of hold of a key.
    • may not apply to initial (base) tap, but will to all subsequent taps.
  • deadkey restoration when restoring context for each multitap key rule evaluation
  • decent key-correction distributions for use in predictive-text corrections.

And, with this... we should actually be able to start proper user testing & regression testing; we should be at full feature-parity with KMW 16.0 and its OSK, with the addition of other keys being able to request multitaps. So...

User Testing

TEST_DOUBLETAP_CAPS: Using the standard "Test unminified KeymanWeb" test page from a mobile device...

  1. Select "Norwegian" - you should then see "Norwegian - EuroLatin (SIL)" in the space bar.
  2. Type All .
  3. Double-tap the shift key. Afterward, the shift key should be highlighted, and its up-arrow should have a thin line underneath, as if underlining it.
  4. Without touching the shift key at any point, type CAPS .
    • If this is impossible - i.e., the layer shifted out from underneath you - FAIL the test.
  5. Single-tap the shift key. Afterward, you should be returned to the base layer.
  6. Type work .
  7. Tap the shift key once.
  8. Without touching the shift key at any point, type Well.
    • If this is impossible - i.e., the layer did not drop the the base layer when expected - FAIL the test.
    • Expected final result, in total: All CAPS work Well

TEST_10_KEY_ROTA: Using the standard "Test unminified KeymanWeb" test page from a mobile device...

  1. Select "English...", then "Classic 10-key".
  2. Verify that all of the keys numbered 1-9 aside from 7 have three or more lowercase letters as key hints.
  3. Tap the 8 key once, then wait for a second.
  4. Tap the 8 key twice in quick succession, then wait for a second.
    • Expected output: 8a.
  5. Tap the 8 key continuously for at least 2 seconds.
    • it should rotate through: 8a8, 8aa, 8ab, 8ac, 8aA, 8aB, 8aC, then back to 8a8 (and continue from there)
    • stop tapping when the new character is a capital letter.
  6. Verify that the key hints are now capitalized.
  7. Double-tap the 9 key once. A D should be emitted as the result.
  8. Repeatedly tap 9 for at least five seconds, paying attention to the key hints.
    • Whenever a lowercase letter is the result, the key hints should be lowercase
    • Whenever an uppercase letter is the result, the key hints should be uppercase

TEST_10_KEY_DIACRITICS: Using the standard "Test unminified KeymanWeb" test page from a mobile device...

  1. Select "English...", then "Diacritic 10-key Rota".
  2. Tap the '◌̀' key. (The key between backspace [above] and enter [below], representing a diacritic)
  3. Double-tap the 8 key.
    • Expected result: à
  4. Triple-tap the '◌̀' key - a circumflex diacritic should appear over the à.
  5. Triple-tap the 9 key.
    • Expected total result: àê
  6. Tap the '◌̀' key. A diacritic should appear over the ê
  7. Tap the 4 key once.
  8. Hit backspace.
  9. Double-tap the 8 key.
    • Expected final result: àềa
      image
    • For clarity:
      • With a copy of the à's accent-mark on top the original ê, as in the cropped screenshot above.
        • It looks different in raw-text form here due to GitHub's font / rendering.
      • Note: there is no matching accent-mark on the second a.

TEST_CUSTOM_MULTITAP_MODIFIER: Using the standard "Test unminified KeymanWeb" test page from a mobile device...

  1. Select "English...", then "Gesture Prototyping".
  2. Single-tap the ◌́ key. (The key between the spacebar [left] and the Enter key [right].)
  3. A number of letters, including all of the vowels, should change to versions using the ◌́ accent mark.
  4. Tap the à and verify that the corresponding letter is output.
  5. Single-tap the abc key (which has replaced the ◌́ on the active layer), returning to the default layer, then wait for at least a second.
  6. Note the hints on the ◌́, then triple-tap it.
  7. A number of letters, including all of the vowels, should change to versions using the ◌̂ mark
    • This corresponds to the second diacritic from the ◌́-key's hint.
  8. Tap the â and verify that the corresponding letter is output.

TEST_MISCELLANEOUS: If anything you consider an error or bug occurs that seems unrelated to the tests, please note it here.

  • At this point in gesture-development, all KMW 16.0 functionality should be fully supported. If you see something you'd normally report as a bug when regression testing, FAIL this "test" and document the bug here.
  • An actual regression-test run will be requested later on, so don't feel the need to perform a full regression-test set at this time.

@keymanapp-test-bot keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label Oct 11, 2023
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Oct 11, 2023

User Test Results

Test specification and instructions

  • TEST_DOUBLETAP_CAPS (PASSED): Tested in Android Mobile device (Samsung Galaxy A23 5g) using the standard "Test unminified KeymanWeb" test page and here is my observation: 1. Set "Norwegian -EuroLatin (SIL)" keyboard. 2. Entered "All". 3. Double-tapped the shift key. Verified that the shift key was highlighted and its up-arrow appeared with a thin line underlining it. 4. Without touching the shift key at any point, type 'CAPS'. 5. Single-tapped the shift key. Verified that the keyboard keys switched back to the base layer. 6. Typed 'work'. Tapped the shift key once. 7. Without touching the shift key at any point, typed 'Well'. Verified that the expected result appears in total: ALL CAPS work Well. Seems to be working as expected. (notes)
  • TEST_10_KEY_ROTA (PASSED): Tested in Android Mobile device (Samsung Galaxy A23 5g) using the standard "Test unminified KeymanWeb" test page and here is my observation: 1. Set "English - Classic 10-key" keyboard. 2. Verified that all of the key numbers 1-9 aside from 7 have three or more lowercase letters as key hints. 3. Tapped the 8 key once, then tapped the 8 key twice in quick succession. Verified that it outputs '8a' on the text input screen. 4. Tapped the 8 key continuously for at least 2 seconds and verified that it outputs '8a8'. 5. Tried to see similar outputs '8aa, 8ab, 8ac, 8aA, 8aB, 8aC, and I was able to see those alphanumeric characters on the screen. Stopped tapping when the typed character appears in caps. 6. Verified that the key hits are now capitalized. 7. Double-tapped the 9 key once. Verified that the letter 'D' appears as the result. 8. Repeatedly, typed '9' key for at least five seconds. Verified that whenever the lowercase letter is the result, the key hint appears in lowercase. And whenever an uppercase letter is the result, the key hints are in uppercase. (notes)
  • TEST_10_KEY_DIACRITICS (PASSED): Tested in Android Mobile device (Samsung Galaxy A23 5g) using the standard "Test unminified KeymanWeb" test page and here is my observation: 1. Set "English - Diacritic 10-key Rota" keyboard. 2. Tapped the '◌̀'' key. 3. Double tapped the 8 key. Verified that the output result à on the screen. 4. Triple-tapped the '◌̀'. 5. Triple-tapped the 9 key. Verified that the total result outputs 'àê'. 6. Tap the '◌̀' key. Verified that the diacritic appears as ê. 7. Tapped the 4 key once. Hit the backspace. 8. Double-tapped the 8 key. Verified that the result shows 'àềa' in the text screen. Tested in Android Mobile device (Samsung Galaxy A23 5g) using the standard "Test unminified KeymanWeb" test page and here is my observation: 1. Set "English - Diacritic 10-key Rota" keyboard. 2. Tapped the '◌̀'' key. 3. Double tapped the 8 key. Verified that the output result à on the screen. 4. Triple-tapped the '◌̀'. 5. Triple-tapped the 9 key. Verified that the total result outputs 'àê'. 6. Tapped the '◌̀' key. Verified that the diacritic appears as ê. 7. Tapped the 4 key once. Hit the backspace. 8. Double-tapped the 8 key. Verified that the result shows 'àềa' in the text screen. (notes)
  • TEST_CUSTOM_MULTITAP_MODIFIER (PASSED): Tested in Android Mobile device (Samsung Galaxy A23 5g) using the standard "Test unminified KeymanWeb" test page and here is my observation: 1. Set "English - Gesture Prototyping" keyboard. 2. Single-tapped the ◌́ key. Verified that the number of letters, including all of the vowels changed to version using the ◌́ accent mark. 3. Tapped the à letter and verified that the corresponding letter is output on the screen. 4. Single-tapped the abc key and noticed that it returned to the default layer. 5. Triple-tapped ◌́, and verified that the corresponding letters showed the second diacritic from the ◌́, hint. 6. Tapped the â key and verified that the corresponding letter shows on the text screen. (notes)
  • TEST_MISCELLANEOUS (PASSED): Got an error message while trying to set 'English - Classic 10-key' keyboard in the Android mobile (TEST_10_KEY_ROTA). Tried to reproduce this error message but seems to be a random one. Anyhow, I will keep an eye on this. I did not observe any other issues apart from this. As per your suggestion, it would be better to add this test to our regression suite (in Testlodge) (notes)

Test Artifacts

static dfltShiftMultitap: LayoutSubKey = {
// Needs to be something special and unique. Typing restricts us from
// using a reserved key-id prefix, though.
id: "T_MT_SHIFT_TO_CAPS",
Copy link
Contributor Author

@jahorton jahorton Oct 12, 2023

Choose a reason for hiding this comment

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

I would love to give this a truly unique identifier... perhaps something prefixed with S_ (as in, "special") or I_ (as in, "internal")... but the strict typing currently in place in common/web/types prohibits that.

type Key_Type = 'T'|'K'|'U'|'t'|'k'|'u';
type Key_Id = string;
export type TouchLayoutKeyId = `${Key_Type}_${Key_Id}`; // pattern = /^[TKUtku]_[a-zA-Z0-9_]+$/

While Web doesn't have to be completely bound to the typing there... the typing, as is, is not particularly adaptable. I'd have to redefine nearly all the touch-layout types within Web to permit use of the extra key ID prefix were that to be the way forward. Same issue arises if we were to signal that id may be undefined or ''.

Alternatively, I could just... not preprocess the key and dynamically rebuild the subkey array every time at runtime... but I'm not convinced that's the best idea.

Copy link
Member

Choose a reason for hiding this comment

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

The typing is there for a reason: it helps us avoid introducing hidden private use types of identifiers in any of the various places these identifiers are used. Leaky abstractions and all that. Yeah, exactly like this case! So this time, at least, the typing is doing its job.

All that said, I'm really happy to look at sensible ways to extend the typespace for private use. I am hesitant to add a new Key_Type because we'd need to audit all the uses to make sure we don't accidentally break e.g. validation in the compiler. But given the Key_Id is limited to an alnum_ pattern, couldn't we do something as simple as T_*_MT_SHIFT_TO_CAPS?

Then all I'd ask is that we add a comment to the keyman-touch-layout-file.ts explaining this private use.

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 I should also add that this type is an input type for a data structure that's meant to be very rigid. We never want to see undefined or S_foo in .keyman_touch_layout files. So it wouldn't be appropriate to be extending the types for the input data structures, rather it would be appropriate for KeymanWeb to have its own 'subclassing' of the structures and types to avoid mixing implementation and data details.

@mcdurdin mcdurdin modified the milestones: A17S23, A17S24 Oct 15, 2023
Base automatically changed from feat/web/longpress-restoration to feature-gestures October 20, 2023 09:21
@jahorton jahorton force-pushed the feat/web/generalized-multitap branch from bba5473 to b669201 Compare October 24, 2023 06:33
@jahorton jahorton changed the base branch from feature-gestures to feat/web/test-kbd-prototyping October 24, 2023 06:34
@jahorton jahorton force-pushed the feat/web/generalized-multitap branch from 3d6e2be to f8ae8c9 Compare October 26, 2023 06:31
@mcdurdin mcdurdin modified the milestones: A17S24, A17S25 Oct 27, 2023
@keymanapp-test-bot keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed and removed user-test-missing User tests have not yet been defined for the PR labels Nov 1, 2023
@jahorton jahorton marked this pull request as ready for review November 1, 2023 06:14
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. I like the way that you've implemented this. Particularly the clean and declarative approach to handling shift->caps multitap.

Base automatically changed from feat/web/test-kbd-prototyping to feature-gestures November 6, 2023 01:27
@bharanidharanj
Copy link

Test Results

  • TEST_DOUBLETAP_CAPS (PASSED): Tested in Android Mobile device (Samsung Galaxy A23 5g) using the standard "Test unminified KeymanWeb" test page and here is my observation: 1. Set "Norwegian -EuroLatin (SIL)" keyboard. 2. Entered "All". 3. Double-tapped the shift key. Verified that the shift key was highlighted and its up-arrow appeared with a thin line underlining it. 4. Without touching the shift key at any point, type 'CAPS'. 5. Single-tapped the shift key. Verified that the keyboard keys switched back to the base layer. 6. Typed 'work'. Tapped the shift key once. 7. Without touching the shift key at any point, typed 'Well'. Verified that the expected result appears in total: ALL CAPS work Well. Seems to be working as expected.

@bharanidharanj
Copy link

bharanidharanj commented Nov 6, 2023

  • TEST_10_KEY_ROTA (PASSED): Tested in Android Mobile device (Samsung Galaxy A23 5g) using the standard "Test unminified KeymanWeb" test page and here is my observation: 1. Set "English - Classic 10-key" keyboard. 2. Verified that all of the key numbers 1-9 aside from 7 have three or more lowercase letters as key hints. 3. Tapped the 8 key once, then tapped the 8 key twice in quick succession. Verified that it outputs '8a' on the text input screen. 4. Tapped the 8 key continuously for at least 2 seconds and verified that it outputs '8a8'. 5. Tried to see similar outputs '8aa, 8ab, 8ac, 8aA, 8aB, 8aC, and I was able to see those alphanumeric characters on the screen. Stopped tapping when the typed character appears in caps. 6. Verified that the key hits are now capitalized. 7. Double-tapped the 9 key once. Verified that the letter 'D' appears as the result. 8. Repeatedly, typed '9' key for at least five seconds. Verified that whenever the lowercase letter is the result, the key hint appears in lowercase. And whenever an uppercase letter is the result, the key hints are in uppercase.

..Able to see the Alphanumeric characters

..Able to see the key hits changed to upper or lower character according to the key results

@bharanidharanj
Copy link

  • TEST_10_KEY_DIACRITICS (PASSED): Tested in Android Mobile device (Samsung Galaxy A23 5g) using the standard "Test unminified KeymanWeb" test page and here is my observation: 1. Set "English - Diacritic 10-key Rota" keyboard. 2. Tapped the '◌̀'' key. 3. Double tapped the 8 key. Verified that the output result à on the screen. 4. Triple-tapped the '◌̀'. 5. Triple-tapped the 9 key. Verified that the total result outputs 'àê'. 6. Tap the '◌̀' key. Verified that the diacritic appears as ê. 7. Tapped the 4 key once. Hit the backspace. 8. Double-tapped the 8 key. Verified that the result shows 'àềa' in the text screen. Tested in Android Mobile device (Samsung Galaxy A23 5g) using the standard "Test unminified KeymanWeb" test page and here is my observation: 1. Set "English - Diacritic 10-key Rota" keyboard. 2. Tapped the '◌̀'' key. 3. Double tapped the 8 key. Verified that the output result à on the screen. 4. Triple-tapped the '◌̀'. 5. Triple-tapped the 9 key. Verified that the total result outputs 'àê'. 6. Tapped the '◌̀' key. Verified that the diacritic appears as ê. 7. Tapped the 4 key once. Hit the backspace. 8. Double-tapped the 8 key. Verified that the result shows 'àềa' in the text screen.

..output 1

..output 2

..output 3

@bharanidharanj
Copy link

  • TEST_CUSTOM_MULTITAP_MODIFIER (PASSED): Tested in Android Mobile device (Samsung Galaxy A23 5g) using the standard "Test unminified KeymanWeb" test page and here is my observation: 1. Set "English - Gesture Prototyping" keyboard. 2. Single-tapped the ◌́ key. Verified that the number of letters, including all of the vowels changed to version using the ◌́ accent mark. 3. Tapped the à letter and verified that the corresponding letter is output on the screen. 4. Single-tapped the abc key and noticed that it returned to the default layer. 5. Triple-tapped ◌́, and verified that the corresponding letters showed the second diacritic from the ◌́, hint. 6. Tapped the â key and verified that the corresponding letter shows on the text screen.

@bharanidharanj
Copy link

Test Results

  • TEST_MISCELLANEOUS (PASSED): Got an error message while trying to set 'English - Classic 10-key' keyboard in the Android mobile (TEST_10_KEY_ROTA). Tried to reproduce this error message but seems to be a random one. Anyhow, I will keep an eye on this. I did not observe any other issues apart from this. As per your suggestion, it would be better to add this test to our regression suite (in Testlodge)

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Nov 6, 2023
@mcdurdin mcdurdin modified the milestones: A17S25, A17S26 Nov 13, 2023
@jahorton jahorton merged commit 6d68df3 into feature-gestures Nov 13, 2023
2 checks passed
@jahorton jahorton deleted the feat/web/generalized-multitap branch November 13, 2023 04:02
@jahorton jahorton mentioned this pull request Dec 12, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants