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(developer): Rework Touch Layout Editor to support flick and multitap 🙊 #6884

Merged
merged 23 commits into from
Jul 25, 2022

Conversation

mcdurdin
Copy link
Member

@mcdurdin mcdurdin commented Jul 4, 2022

Blocked: this is blocked by #6918, will want to run the regression test before this lands, so once #6918 chain hits master, we should rebase/merge master. DONE.


This is a significant rework of the Touch Layout Editor. I started by refactoring the code into multiple units with minimal code changes -- you can see these changes in the following commits:

  • e6a321d - chore(developer): refactor builder.js
  • 2743574 - chore(developer): refactor undo actions out of builder.js
  • f6974e0 - chore(developer): refactor 'constants' out of builder.js
  • 84d8f26 - chore(developer): refactor subkeys out of builder.js
  • 4578742 - chore(developer): update jquery-ui version to 1.13.1

I did make some more minor further refactoring changes in subsequent commits, but did not separate them out quite as much. However, first reviewing the above commits separately, and then reviewing the remaining commits in a block should make it easier to see what's going on.

This PR includes the following feature work and bug fixes:

  • Restructure touch layout editor to make space for additional gesture types:
    • Move control bar to right hand side
    • Move key cap edit controls to control bar (instead of being on the key)
  • Adds support for flick and multitap gestures.
  • New types of buttons to add longpress keys, flicks or multitaps (removing all gestures of a given type deletes the entire gesture object in the JSON).
    • Note that the 'wedge' style of adding keys is still available on longpress and on multitap
  • Adds Unicode value boxes which mirror the key cap text box.
  • Maintain selected key as far as possible across layer switch, platform switch and presentation view switch.
  • Split 'special' key cap icons from normal key caps in the controls, so users can pick them visually.
  • Updates key width values in real time when resizing.
  • Fixes selection of last active element with undo

And finally, also updated relevant Delphi code to allow the new fields to validate successfully.

image

There are a number of changes to the UX. First, it has been significantly rearranged to simplify editing different gesture types. These rearrangements should also improve the editing experience for any touch keyboard, even if new gesture types are not used.

Controls for editing

The controls for editing key metadata are all to the right of the keyboard view -- even the key cap is edited here now:

image

There are a few new fields here:

Keycap Value

This allows the user to choose keycap images visually rather than by using the previous *<name>* pattern which was a little obtuse (and was hard to work with in the keycap text field as the names didn't really fit):

image

Note that when a 'special' key cap is selected, the Text and Unicode fields are hidden:

image

Unicode

The Unicode field lets the user see and/or enter Unicode values directly for the key Text.

image

Gesture Type

This field is read-only but correlates to the selected key area (the same information is available visually by the location of the key).

image

Gesture indicators

The slash on the top right of a key that indicated the presence of a longpress menu is gone. In its place we have the following indicators that indicate the presence of a gesture on the key, located in rough association with the direction of the gesture (where appropriate):

  • Longpress: an icon at the top of the key:
    image

  • Flicks: one icon for each flick direction defined for the key:
    image

  • Multitaps: an icon to the bottom right of the key:
    image

Note: these icons are only visible in the designer -- not in the running keyboard. We are still planning to introduce 'hints' for key caps; when we implement this, it may impact these icons.

User Testing

In all your testing, it will be valuable to have the F12 Developer Tools window open to verify that there are no errors raised.

For every test, we want to make sure that (a) no errors occur, and (b) the result is what you would expect to have happen.

SUITE_BASIC

Here, we aim to test that touch layout designer functionality available in earlier versions still works in the new version.

  • TEST_ADD_KEY: Add a key to an existing row of the keyboard (with the green 'wedge').
  • TEST_ADD_ROW: Add a new row to an existing layer on the keyboard (with the green 'wedge').
  • TEST_ADD_LAYER: Add a new layer to a keyboard (Layer/Add button)
  • TEST_DEL_KEY: Delete a key from the keybaord (Red x button on the selected key)
  • TEST_DEL_ROW: Delete a row from the keyboard by deleting all keys in the row.
  • TEST_DEL_LAYER: Delete a layer from the keyboard (Layer/Del button).
  • TEST_ADD_PLATFORM: Add either tablet or phone platform.
  • TEST_DEL_PLATFORM: Delete either tablet or phone platform from the keyboard.
  • TEST_TEMPLATE: Import from On Screen, then use the Template button to select the Basic layout, and verify that this generates a 'basic' layout -- mobile-optimised, fewer keys than a desktop layout.
  • TEST_SWITCH_LAYER: Select an alternative layer from the Layer menu. Then double-click on the Shift key to switch layers another way.
  • TEST_PRESENTATIONS: Switch between the landscape/portrait orientation and verify correct.

SUITE_EDIT_KEY_DATA

Next, we'll want to test the key cap editing behaviour.

  • TEST_EDIT_TEXT: Select a key and change its key cap value in the Text field.
  • TEST_EDIT_UNICODE: Select a key and change its key cap value in the Unicode field.
  • TEST_EDIT_ID: Select a key and change its ID.
  • TEST_EDIT_PAD: Select a key and change the padding to a value such as 30. Verify that the padding left of the key grows.
  • TEST_EDIT_WIDTH: Select a key and change its width to the value 200. Verify that the key is twice as wide as normal.

SUITE_EDIT_GESTURE_DATA:

  • TEST_ADD_LONGPRESS: For a selected base key, add a longpress by clicking the [+] button in the Longpress area. Verify that the longpress icon appears on the base key.
  • TEST_DEL_LONGPRESS: For a selected base key, delete a longpress key by clicking the red (x) button on the longpress key.
  • TEST_ADD_FLICK: For a selected base key, add a flick by clicking any [+] button in the Flick area. Verify that the coresponding Flick icon appears on the base key.
  • TEST_DEL_FLICK: For a selected base key, delete a flick key by clicking the red (x) button on the flick key.
  • TEST_ADD_MULTITAP: For a selected base key, add a multitap by clicking the [+] button in the Multitap area. Verify that the multitap icon appears on the base key.
  • TEST_DEL_MULTITAP: For a selected base key, delete a multitap key by clicking the red (x) button on the multitap key.

SUITE_CHAR_MAP

  • TEST_CHAR_DROP: Drag a character from character map and drop it onto a key. The new character should replace the character on the key.
  • TEST_CHAR_DROP_ADD: Drag a character from character map and drop it onto a key while holding Shift. The new character should be added to the key.
  • TEST_CHAR_DROP_TEXT: Drag a character from character map and drop it onto the Text field. The new character should replace the character on the key.
  • TEST_CHAR_DROP_TEXT_ADD: Drag a character from character map and drop it onto the Text field while holding Shift. The new character should be added to the key.

SUITE_USAGE

  • TEST_GENERAL_USE: It would be valuable to just give the touch layout editor some general use, playing around with it, and verifying that it behaves as you expect, as well as all the above tests. Any behaviour that doesn't "make sense" or causes an error would be a reason to fail this test.

@mcdurdin mcdurdin requested a review from darcywong00 as a code owner July 4, 2022 00:48
@mcdurdin mcdurdin added this to the A16S5 milestone Jul 4, 2022
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Jul 4, 2022

User Test Results

Test specification and instructions

✅ SUITE_BASIC:

11 tests in 1 groups PASSED
  • TEST_ADD_KEY (PASSED): Tested this in Keyman Developer 16.0.20 build (with amharic Keyboard) and it is working as expected. (notes)
  • TEST_ADD_ROW (PASSED): Tested this in Keyman Developer 16.0.20 build (with amharic Keyboard) and it is working as expected. (notes)
  • TEST_ADD_LAYER (PASSED): Tested this in Keyman Developer 16.0.20 build (with amharic Keyboard) and it is working as expected. (notes)
  • TEST_DEL_KEY (PASSED): Tested this in Keyman Developer 16.0.20 build (with amharic Keyboard) and it is working as expected.
  • TEST_DEL_ROW (PASSED): Tested this in Keyman Developer 16.0.20 build (with amharic Keyboard) and it is working as expected. (notes)
  • TEST_DEL_LAYER (PASSED): Tested this in Keyman Developer 16.0.20 build (with amharic Keyboard) and it is working as expected.
  • TEST_ADD_PLATFORM (PASSED): Tested this in Keyman Developer 16.0.20 build (with amharic Keyboard) and it is working as expected.
  • TEST_DEL_PLATFORM (PASSED): Tested this in Keyman Developer 16.0.20 build (with amharic Keyboard) and it is working as expected.
  • TEST_TEMPLATE (PASSED): I ran through this test myself and am confident it is working correctly now.
  • TEST_SWITCH_LAYER (PASSED): Tested this in Keyman Developer 16.0.20 build (with amharic Keyboard) and it is working as expected.
  • TEST_PRESENTATIONS (PASSED): Tested this in Keyman Developer 16.0.20 build (with amharic Keyboard) and it is working as expected. (notes)

✅ SUITE_EDIT_KEY_DATA:

5 tests in 1 groups PASSED
  • TEST_EDIT_TEXT (PASSED): Able to change the Key cap value in the Text field. (notes)
  • TEST_EDIT_UNICODE (PASSED): Able to change the Key cap value in the Unicode field. (notes)
  • TEST_EDIT_ID (PASSED): Able to change the Key's ID. (notes)
  • TEST_EDIT_PAD (PASSED): Able to see the paddinf left of the key grows. (notes)
  • TEST_EDIT_WIDTH (PASSED): Able to see the key is twice as wide as normal after changing the value of the width to 200. (notes)

✅ SUITE_EDIT_GESTURE_DATA:

6 tests in 1 groups PASSED
  • TEST_ADD_LONGPRESS (PASSED): Verified that the Longpress icon appears on the base key. (notes)
  • TEST_DEL_LONGPRESS (PASSED): Verified that the longpress icone has been removed from the base key after clicking the red (x) button on the longpress key. (notes)
  • TEST_ADD_FLICK (PASSED): Verified that the corresponding Flick icon appears onthe base key. (notes)
  • TEST_DEL_FLICK (PASSED): Verified that the Flick key has been removed from the base key after clicking the red (x) button on the Flick key. (notes)
  • TEST_ADD_MULTITAP (PASSED): Retested this as per Mar'c suggestion and yes, I was able to see the Multitap icon appears on the Key. (but partially visible) (notes)
  • TEST_DEL_MULTITAP (PASSED): Retested this and I noticed that after deleting the Multitap key, the corresponding icon has been removed from the Key. (notes)

✅ SUITE_CHAR_MAP:

4 tests in 1 groups PASSED
  • TEST_CHAR_DROP (PASSED): Verified that the new character was replaced the character on the key. (notes)
  • TEST_CHAR_DROP_ADD (PASSED): Verified that the new character was added to the key. (notes)
  • TEST_CHAR_DROP_TEXT (PASSED): Verified that the new character was replaced the character on the key. (notes)
  • TEST_CHAR_DROP_TEXT_ADD (PASSED): Verified that the new character was added to the key. (notes)

✅ SUITE_USAGE:

1 tests in 1 groups PASSED
  • TEST_GENERAL_USE (PASSED): I have done some general testing in the Touch Layout editor with all the options that are available in the Touch layout window. Seems to be working fine. However, I noticed that I got an compilation error message after adding a 'LongPress' feature to a base Key. (notes)

Test Artifacts

@keymanapp-test-bot keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label Jul 4, 2022
@mcdurdin mcdurdin marked this pull request as draft July 4, 2022 00:48
@mcdurdin mcdurdin marked this pull request as ready for review July 5, 2022 04:37
@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 Jul 5, 2022
@mcdurdin mcdurdin force-pushed the feat/developer/touch-layout-editor-flick-and-multitap branch from afa20aa to 5e88939 Compare July 5, 2022 04:46
@bharanidharanj
Copy link

SUITE_BASIC:

  • TEST_ADD_KEY (PASSED): Tested this in Keyman Developer 16.0.20 build (with amharic Keyboard) and it is working as expected.

  • TEST_ADD_ROW (PASSED): Tested this in Keyman Developer 16.0.20 build (with amharic Keyboard) and it is working as expected.

  • TEST_ADD_LAYER (PASSED): Tested this in Keyman Developer 16.0.20 build (with amharic Keyboard) and it is working as expected.

  • TEST_DEL_KEY (PASSED): Tested this in Keyman Developer 16.0.20 build (with amharic Keyboard) and it is working as expected.

  • TEST_DEL_ROW (PASSED): Tested this in Keyman Developer 16.0.20 build (with amharic Keyboard) and it is working as expected.

  • TEST_DEL_LAYER (PASSED): Tested this in Keyman Developer 16.0.20 build (with amharic Keyboard) and it is working as expected.
  • TEST_ADD_PLATFORM (PASSED): Tested this in Keyman Developer 16.0.20 build (with amharic Keyboard) and it is working as expected.
  • TEST_DEL_PLATFORM (PASSED): Tested this in Keyman Developer 16.0.20 build (with amharic Keyboard) and it is working as expected.
  • TEST_TEMPLATE (PASSED): Tested this in Keyman Developer 16.0.20 build (with amharic Keyboard) and it is working as expected.
  • TEST_SWITCH_LAYER (PASSED): Tested this in Keyman Developer 16.0.20 build (with amharic Keyboard) and it is working as expected.
  • TEST_PRESENTATIONS (PASSED): Tested this in Keyman Developer 16.0.20 build (with amharic Keyboard) and it is working as expected.

@bharanidharanj
Copy link

SUITE_EDIT_KEY_DATA:

  • TEST_EDIT_TEXT (PASSED): Able to change the Key cap value in the Text field.

  • TEST_EDIT_UNICODE (PASSED): Able to change the Key cap value in the Unicode field.

  • TEST_EDIT_ID (PASSED): Able to change the Key's ID.

  • TEST_EDIT_PAD (PASSED): Able to see the paddinf left of the key grows.

  • TEST_EDIT_WIDTH (PASSED): Able to see the key is twice as wide as normal after changing the value of the width to 200.

@bharanidharanj
Copy link

SUITE_EDIT_GESTURE_DATA:

  • TEST_ADD_LONGPRESS (PASSED): Verified that the Longpress icon appears on the base key.
    LongPressKey1

  • TEST_DEL_LONGPRESS (PASSED): Verified that the longpress icone has been removed from the base key after clicking the red (x) button on the longpress key.

DelLongpress

  • TEST_ADD_FLICK (PASSED): Verified that the corresponding Flick icon appears onthe base key.

AddFlick

  • TEST_DEL_FLICK (PASSED): Verified that the Flick key has been removed from the base key after clicking the red (x) button on the Flick key.

DelFlickKey

  • TEST_ADD_MULTITAP (FAILED): Noticed that the multitap icon does not appear on the base key.

AddMultitap

  • TEST_DEL_MULTITAP (FAILED): Noticed that the this functionality is not working as expected.

DelMultitap

@bharanidharanj
Copy link

SUITE_CHAR_MAP:

  • TEST_CHAR_DROP (PASSED): Verified that the new character was replaced the character on the key.

Drag1

  • TEST_CHAR_DROP_ADD (PASSED): Verified that the new character was added to the key.

Drag2

  • TEST_CHAR_DROP_TEXT (PASSED): Verified that the new character was replaced the character on the key.

Drag3

  • TEST_CHAR_DROP_TEXT_ADD (PASSED): Verified that the new character was added to the key.

Drag4

@mcdurdin
Copy link
Member Author

mcdurdin commented Jul 5, 2022

  • TEST_ADD_MULTITAP (FAILED): Noticed that the multitap icon does not appear on the base key.

I think the multitap icon is present, perhaps not visible enough?

image

  • TEST_DEL_MULTITAP (FAILED): Noticed that the this functionality is not working as expected.

Can you let me know what is not working as you expected? Is it just related to the multitap icon from the previous test?

@keymanapp-test-bot retest SUITE_EDIT_GESTURE_DATA TEST_ADD_MULTITAP TEST_DEL_MULTITAP

Copy link
Contributor

@jahorton jahorton left a comment

Choose a reason for hiding this comment

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

Does this PR include code to actively check against the new schema(s), even if only for linting purposes?

Base automatically changed from feat/developer/touch-layout-flick-and-multitap to master July 6, 2022 05:18
@MakaraSok
Copy link
Collaborator

@mcdurdin After playing with SUITE_BASIC I've noticed one thing that I expect to see.

When delete something from Touch Layout tab, i.e. a layer or a platform, I expect a warning pop-up message telling what's gonna happen. Otherwise, the current layer would just disappear as soon as the Del button is pressed. I know the undo key could help getting the deleted layer back, but it would be nice to give a warning message. :)

@bharanidharanj
Copy link

  • TEST_ADD_MULTITAP (FAILED): Noticed that the multitap icon does not appear on the base key.

I think the multitap icon is present, perhaps not visible enough?

image

  • TEST_DEL_MULTITAP (FAILED): Noticed that the this functionality is not working as expected.

Can you let me know what is not working as you expected? Is it just related to the multitap icon from the previous test?

@keymanapp-test-bot retest SUITE_EDIT_GESTURE_DATA TEST_ADD_MULTITAP TEST_DEL_MULTITAP

@mcdurdin I thought since Multitap feature is not working, so that I failed this test too. Anyhow, I will re-test and add my comment. Thanks.

@bharanidharanj
Copy link

SUITE_EDIT_GESTURE_DATA:

  • TEST_ADD_MULTITAP (PASSED): Retested this as per Mar'c suggestion and yes, I was able to see the Multitap icon appears on the Key. (but partially visible)
    AddMultitap

  • TEST_DEL_MULTITAP (PASSED): Retested this and I noticed that after deleting the Multitap key, the corresponding icon has been removed from the Key.
    DelMultitap

@bharanidharanj
Copy link

SUITE_USAGE:

  • TEST_GENERAL_USE (PASSED): I have done some general testing in the Touch Layout editor with all the options that are available in the Touch layout window. Seems to be working fine. However, I noticed that I got an compilation error message after adding a 'LongPress' feature to a base Key.

Note: I did not see any error message while adding Flicks Key or Multitap key after compilation.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Jul 6, 2022
@MakaraSok
Copy link
Collaborator

MakaraSok commented Jul 6, 2022

Test Results

SUITE_BASIC:

  • TEST_TEMPLATE (FAILED): I observe that this TEST_TEMPLATE is weird when you change the template to template-basic.keyman-touch-layout. The keyboard layout gets replaced by the English layout instead of simplifying the current layout. To make the matter worse, the action cannot be reversed. The "undo" button is grayed out.
Screen.Recording.2022-07-06.at.2.53.26.PM.mov

When change the template on the existing layout, the template replace the layout nicely. See below.

change.template.on.existing.layout.mov

Other observations:

  1. There is ZWSP in the Keycap Value, but there are several ZWNJ.

image

  1. Can we have a consistent measure of font size everywhere, i.e. OSK, TLP, TLT and TLD? What would be more familiar to me personally is pt instead of em. 3. See the screenshot below.

image

@@ -191,7 +191,7 @@ procedure TAppSourceHttpResponder.RespondTouchEditorLib(
begin
doc := ARequestInfo.Document;
Delete(doc, 1, Length('/app/source/toucheditor/lib/'));
if (Pos('/', doc) > 0) or (Pos('\', doc) > 0) then
if Pos('..', doc) > 0 then
Copy link
Member Author

Choose a reason for hiding this comment

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

We do allow sub-paths, just not parent navigation -- this fixes loading of jquery-ui images.

@@ -36,7 +36,7 @@ class function TKeymanDeveloperPaths.NodePath: string;
KeymanRoot: string;
begin
if TKeymanPaths.RunningFromSource(KeymanRoot)
then Result := KeymanRoot + 'developer\src\inst\dist\node.exe'
then Result := KeymanRoot + 'developer\src\inst\node\dist\node.exe'
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixup for node loading when running Developer from within a debugger in the source path.

mcdurdin added 8 commits July 14, 2022 10:54
* Restrucure touch layout editor to make space for additional gesture
  types:
  * Move control bar to right hand side
  * Move key cap edits to control bar.
* Adds flick and multitap gestures:
  * Adds buttons to add longpress keys, flicks or multitaps.
* Adds Unicode value boxes which mirror the key cap text box.
* Updates key width values in real time when resizing
* Maintain selected key as far as possible across layer switch, platform
  switch and presentation view switch.
* Starts to split 'special' key cap icons from normal key caps.
* Separates out the 'command' icon key cap controls
* Fixes selection of last active element with undo
Also minor tweaks to allow jquery-ui images to load, fixup for broken
file path for node.exe.
@mcdurdin mcdurdin force-pushed the feat/developer/touch-layout-editor-flick-and-multitap branch from 06ed0ad to b702b7d Compare July 14, 2022 00:55
mcdurdin added 8 commits July 14, 2022 10:57
Improves the .keyman-touch-layout clean schema and adds additional
cleanup during load and save in the Touch Layout Editor.

The compiler already does transforms for the relevant fields to the
formats that KeymanWeb is expecting (in particular, width, pad to
string).

This is preparation for adding hint data to the format as well.
Also fixes a couple of minor issues with character map integration and
Unicode text fields.
Moves the top toolbar to the left to make more room, and tidy it up,
and moves various controls into sub-dialogs to make them clearer.

Shortcut buttons to prepopulate the Hint property will come in a
subsequent PR.
Most dialogs in touch layout editor can now have just a Close
button with changes taking effect immediately. This helps users
visualize how the options they've chosen will work.
After a demo of the Touch Layout Editor @jahorton suggested using a
"default hint" model instead of the "display hint" from the design spec.
I liked this and have reworked the design accordingly. With the default
hint model, a source for the hint text can be specified, such as the
default longpress key, or a specific flick key. This can then be
overridden on a key level.

The hint input element now shows a placeholder to indicate the default
hint for the key, and if a hint has been customised for a given key, it
is shown in green on the key cap.

Changing the default hint does not impact any customised hints.
@mcdurdin
Copy link
Member Author

Test Results

SUITE_BASIC:

  • TEST_TEMPLATE (PASSED): I ran through this test myself and am confident it is working correctly now.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Jul 14, 2022
@mcdurdin mcdurdin modified the milestones: A16S6, A16S7 Jul 24, 2022
mcdurdin added 3 commits July 25, 2022 19:57
…chema-high-quality

feat(developer): cleanup touch layout files on load and save 🙊
…ompiler-flick-and-multitap

feat(developer): add support for flick and multitap to compiler 🙊
…out-hints

feat(developer): add hint support to Touch Layout Editor and schema 🙊
@mcdurdin mcdurdin merged commit 5895d26 into master Jul 25, 2022
@mcdurdin mcdurdin deleted the feat/developer/touch-layout-editor-flick-and-multitap branch July 25, 2022 21:14
@sentry-io
Copy link

sentry-io bot commented Nov 21, 2022

Sentry issue: KEYMAN-DEVELOPER-7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common/ developer/ epic A long lived branch, home for a new feature, usually will have child PRs based on it feat has-user-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants