CLDR-17113 kbd: element/attr renaming #3313

srl295 · 2023-10-03T23:07:17Z

layers/@Form > layers/@formid
key/@flicks > key/@flickId
key/@switch > key/@layerId
key/@to > key/@output
display/@to > display/@output
updates to dtd, samples
drop some old data in tests

CLDR-17113

This PR completes the ticket.

ALLOW_MANY_COMMITS=true

srl295 · 2023-10-03T23:07:27Z

needs spec yet

jira-pull-request-webhook · 2023-10-03T23:45:59Z

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

mcdurdin

Minor typo apart from that lgtm

docs/ldml/tr35-keyboards.md

mcdurdin · 2023-10-04T07:57:07Z

docs/ldml/tr35-keyboards.md

-        <key id="a" to="a" longpress="\u{0301} \u{0300}" />
-        <key id="shift" switch="shift" />
+        <key id="a" output="a" longpress="\u{0301} \u{0300}" />
+        <key id="shift" layerId="shift" />


Note: a limitation of longpress is we can't do e.g. longpress="ctrl" to get to the ctrl layer

One way to solve this if needed would be to make the longpress refer to key ids

That would certainly be elegant structurally, but it would then require another lot of key elements to do the mapping to outputs, and those keys would not be able to have longpress themselves, etc. Would require a significant reshuffle of the current design.

They could. Longpress keys could be switching layers, and - if the options are presented in the UI - have multi tap, flicks or more longpress keys (nobody says the UI must dismiss as soon as you touch it) or assigned a display element. Implementations can simply not support it. I am not sure you need that much structural redesign, but also I am not sure how useful this would be and the cost of that is more verbosity in the xml, hence emphasis on "if needed".

Compare this with multi tap situation. In this case, if you wanted to allow layer switching keys or long press keys, then you need significant implementation redesign as things change under your hands, and allowing "nested" multi tap does not make sense in the first place.

My feeling on this is: layering gestures on gestures would lead to significant implementation complexity, a big decrease in usability and discoverability, and very little additional benefit.

As far as I can tell, only the layerId attribute is important as an attribute for any gesture outputs. But if we did refactor in this way, then it would be nice to also specify the longPressDefault as an attribute of the longPress rather than the pattern we have right now <longPress default='true'>.

@DavidLRowe noted that there are extant examples of keyboards in the Keyman keyboards repository that have layerId-type actions assigned to longpress. My quick and somewhat rough search located at least 81 keyboards. There are two kinds of uses:

Keyboards that have a modifier key with additional layer switch keys in the longpress, e.g. koreguaje. As far as I have checked, these are mostly translations from desktop layouts.

Keyboards that emit an output and then swap to another layer. This pattern is widely used, e.g. in naijatype. This is an important usability pattern -- and restricting that ability to base keys seems like a poor outcome.

Given the current restriction we have with CLDR's XML structure, supporting this would lead to significantly more verbose files.

Is it too late to be shuffling these deck chairs?

maybe for consistency, the flick output should also be a key.

That would allow <display> to affect flick output.

Is it too late to be shuffling these deck chairs?

I think the too-late ship has already sailed in a sense, that we're already renaming elements.

I'll update.

jira-pull-request-webhook · 2023-10-04T15:20:48Z

Notice: the branch changed across the force-push!

common/supplemental/attributeValueValidity.xml is no longer changed in the branch
keyboards/dtd/ldmlKeyboard.dtd is no longer changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

docs/ldml/tr35-keyboards.md

jira-pull-request-webhook · 2023-10-09T16:27:23Z

Notice: the branch changed across the force-push!

common/supplemental/attributeValueValidity.xml is no longer changed in the branch
docs/ldml/tr35-keyboards.md is different
keyboards/dtd/ldmlKeyboard.dtd is no longer changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

The base branch was changed.

jira-pull-request-webhook · 2023-10-09T18:23:52Z

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

@Form

- layers/@Form > layers/@formid - key/@flicks > key/@flickId - key/@switch > key/@layerId - key/@to > key/@output - display/@to > display/@output - updates to dtd, samples - drop some old data in tests

- longPress, multiTap now use a new keyList id - keyList is a list of keys, has a default key - flickSegment now uses a keyId instead of output This means that each part of longPress, flick, and multiTap can: - have a <display> customization - perform a layer switch if desired Also, document.

jira-pull-request-webhook · 2023-10-09T21:32:51Z

Notice: the branch changed across the force-push!

docs/ldml/tr35-keyboards.md is different
keyboards/3.0/fr-t-k0-azerty.xml is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

srl295 · 2023-10-10T02:01:01Z

comments from review with @mcdurdin and @rc-swag

drop keyLists and move id lists into attributes
add <key longPressDefault=true
disallow multiTapKeyIds to refer to itself

miloush · 2023-10-10T09:21:16Z

Not sure I quite understand longPressDefault=true, can we see some example usage?

mcdurdin · 2023-10-10T09:31:11Z

Not sure I quite understand longPressDefault=true, can we see some example usage?

It's moving the existing longpressDefault="output-value" property from the parent key to the longpress key itself, where it just becomes a boolean flag.

Further discussion on this change is probably worthwhile:

Pros:

fits more naturally as a property of the longpress key itself now that we can do that, particularly if we define other properties in future
means we don't repeat the key output

Cons:

it would possible to have two longpress keys with default=true in a single longpress list, which requires a lint rule to detect.

miloush · 2023-10-10T10:15:30Z

Maybe this should be done in a separate PR, it's getting a bit crowded.

The PR now has

<keyList id="lower-a" defaultKeyId="a-grave" keyIds="a-grave a-caret a-acute a-umlaut a-tilde a-ring a-caron" />

You said it's moving from key to longpress but the latest comments from @srl295 says key longPressDefault= not longpress default= and also suggests to drop the keyLists altogether?

srl295 · 2023-10-10T13:30:20Z

Maybe I messed up the pr

separate PR

Actually they are very intertwined. I’d rather not create too many , and this change is relevant

srl295 · 2023-10-10T14:34:37Z

Not sure I quite understand longPressDefault=true, can we see some example usage?

OK I see. I just took notes from a review with @mcdurdin and @rc-swag , I didn't actually make the changes yet

miloush · 2023-10-10T16:31:09Z

I see now what was meant. Why is that better than having longPressDefaultKeyId? This change would introduce a requirement that the default long press is in the list of long presses, but if there is only long press default and no long presses, implementation does not need to display any UI and just perform the long-press action.

srl295 · 2023-10-10T16:34:48Z

I see now what was meant. Why is that better than having longPressDefaultKeyId? This change would introduce a requirement that the default long press is in the list of long presses, but if there is only long press default and no long presses, implementation does not need to display any UI and just perform the long-press action.

I can see that— that also means implementations don't need to enforce "only one default".

I think the longPress list of a key should be able to include itself, right?

- replaces longPressDefault=true

srl295 · 2023-10-10T16:43:20Z

I see now what was meant. Why is that better than having longPressDefaultKeyId? This change would introduce a requirement that the default long press is in the list of long presses, but if there is only long press default and no long presses, implementation does not need to display any UI and just perform the long-press action.

@miloush PTAL

srl295 · 2023-10-10T16:44:42Z

also i should have done this before, but i disabled the kbd-check workflow. It won't make sense until I get to re-implement all of these changes.

miloush · 2023-10-10T17:10:11Z

I think the longPress list of a key should be able to include itself, right?

I don't expect keyman to support nested long presses but as spec goes that sounds fine.

srl295 · 2023-10-10T17:11:05Z

I think the longPress list of a key should be able to include itself, right?

I don't expect keyman to support nested long presses but as spec goes that sounds fine.

It's not that the long press is nested. It's that long pressing on a key a can have a as one of the long press choices.

mcdurdin · 2023-10-10T23:56:46Z

I think the longPress list of a key should be able to include itself, right?

I don't expect keyman to support nested long presses but as spec goes that sounds fine.

It's not that the long press is nested. It's that long pressing on a key a can have a as one of the long press choices.

I think the spec should note that implementations are not expected to support nested long press (or other gestures), otherwise authors will depend on this behaviour and be disappointed.

srl295 · 2023-10-11T00:08:37Z

I think the longPress list of a key should be able to include itself, right?

I don't expect keyman to support nested long presses but as spec goes that sounds fine.

It's not that the long press is nested. It's that long pressing on a key a can have a as one of the long press choices.

I think the spec should note that implementations are not expected to support nested long press (or other gestures), otherwise authors will depend on this behaviour and be disappointed.

Noted PTAL

miloush · 2023-10-11T12:42:20Z

docs/ldml/tr35-keyboards.md

@@ -666,6 +666,8 @@ _Attribute:_ `longPressKeyIds="{list of key ids}"` (optional)
 >
 > In a list of keys specified by `longPressKeyIds`, the key matching `longPressDefaultKeyId` attribute (if present) specifies the default long-press target, which could be different than the first element. It is an error if the `longPressDefaultKeyId` key is not in the `longPressKeyIds` list.
 >
+> Gestures are not nested: other gestures (such as flick, multiTap, longPress) are ignored on the keys in the `longPressKeyIds` list.


Like before, do not use "are" and similar factual statements. It is unclear who they target and what they mean. Say "Implementations may ignore/not support" or shall not or whatever is appropriate.

Well put. I will revise.

miloush · 2023-10-11T21:00:24Z

I see you went with an implementation supporting these to be non-conforming... :-)

@Form

Two PRs that were merged to main (45) instead of maint-44 ----- - CLDR-17113 kbd: element/attr renaming (#3313) * CLDR-17113 kbd: element/attr renaming - layers/@Form > layers/@formid - key/@flicks > key/@flickId - key/@switch > key/@layerId - key/@to > key/@output - display/@to > display/@output - updates to dtd, samples - drop some old data in tests * CLDR-17113 kbd display output= and display= should support variables * CLDR-17113 kbd shuffle element deck chairs - longPress, multiTap now use a new keyList id - keyList is a list of keys, has a default key - flickSegment now uses a keyId instead of output This means that each part of longPress, flick, and multiTap can: - have a <display> customization - perform a layer switch if desired Also, document. * CLDR-17113 kbd: move keyList back into attributes * CLDR-17113 kbd: add longPressDefaultKeyId - replaces longPressDefault=true * CLDR-17113 kbd: no nested gestures * CLDR-17113 kbd: no nested gestures,2 (cherry picked from commit 5f1d298) ----- - CLDR-17140 kbd: specify modifier matching (#3314) * CLDR-17140 kbd: specify modifier matching * CLDR-17140 kbd: rename modifier= to modifiers= * CLDR-17140 kbd: modifiers: add 'other' and comma - now supports modifier 'other' if nothing else matches - now supports comma-separated lists of modifier sets - attribute name "modifier" changed to "modifiers" * CLDR-17140 kbd: remove extraneous mention of duplicate keys - per review * CLDR-17140 kbd: modifiers review comments (cherry picked from commit 286a10b)

@Form

Two PRs that were merged to main (45) instead of maint-44 ----- - CLDR-17113 kbd: element/attr renaming (#3313) * CLDR-17113 kbd: element/attr renaming - layers/@Form > layers/@formid - key/@flicks > key/@flickId - key/@switch > key/@layerId - key/@to > key/@output - display/@to > display/@output - updates to dtd, samples - drop some old data in tests * CLDR-17113 kbd display output= and display= should support variables * CLDR-17113 kbd shuffle element deck chairs - longPress, multiTap now use a new keyList id - keyList is a list of keys, has a default key - flickSegment now uses a keyId instead of output This means that each part of longPress, flick, and multiTap can: - have a <display> customization - perform a layer switch if desired Also, document. * CLDR-17113 kbd: move keyList back into attributes * CLDR-17113 kbd: add longPressDefaultKeyId - replaces longPressDefault=true * CLDR-17113 kbd: no nested gestures * CLDR-17113 kbd: no nested gestures,2 (cherry picked from commit 5f1d298) ----- - CLDR-17140 kbd: specify modifier matching (#3314) * CLDR-17140 kbd: specify modifier matching * CLDR-17140 kbd: rename modifier= to modifiers= * CLDR-17140 kbd: modifiers: add 'other' and comma - now supports modifier 'other' if nothing else matches - now supports comma-separated lists of modifier sets - attribute name "modifier" changed to "modifiers" * CLDR-17140 kbd: remove extraneous mention of duplicate keys - per review * CLDR-17140 kbd: modifiers review comments (cherry picked from commit 286a10b)

srl295 self-assigned this Oct 3, 2023

srl295 added the keyboard label Oct 3, 2023

srl295 requested a review from a team October 3, 2023 23:44

srl295 marked this pull request as ready for review October 3, 2023 23:44

srl295 force-pushed the srl295/kbd/cldr-17113-naming branch from f067074 to 85553c4 Compare October 3, 2023 23:45

mcdurdin previously approved these changes Oct 4, 2023

View reviewed changes

srl295 force-pushed the srl295/kbd/cldr-17089-avoid-mixed-children branch from 38b0e70 to c1e301d Compare October 4, 2023 15:19

srl295 force-pushed the srl295/kbd/cldr-17113-naming branch from 85553c4 to 27c1703 Compare October 4, 2023 15:20

miloush reviewed Oct 5, 2023

View reviewed changes

docs/ldml/tr35-keyboards.md Show resolved Hide resolved

srl295 force-pushed the srl295/kbd/cldr-17089-avoid-mixed-children branch from 737e061 to 6ff581a Compare October 9, 2023 16:23

srl295 force-pushed the srl295/kbd/cldr-17113-naming branch from 796a858 to 0e4f793 Compare October 9, 2023 16:27

Base automatically changed from srl295/kbd/cldr-17089-avoid-mixed-children to main October 9, 2023 18:22

srl295 force-pushed the srl295/kbd/cldr-17113-naming branch from 6bac734 to a1a8612 Compare October 9, 2023 18:23

srl295 added 3 commits October 9, 2023 16:32

CLDR-17113 kbd: element/attr renaming

77a7f16

- layers/@Form > layers/@formid - key/@flicks > key/@flickId - key/@switch > key/@layerId - key/@to > key/@output - display/@to > display/@output - updates to dtd, samples - drop some old data in tests

CLDR-17113 kbd display output= and display= should support variables

053d458

srl295 force-pushed the srl295/kbd/cldr-17113-naming branch from f797242 to 24f19c8 Compare October 9, 2023 21:32

CLDR-17113 kbd: move keyList back into attributes

a0ee437

CLDR-17113 kbd: add longPressDefaultKeyId

2ddc18a

- replaces longPressDefault=true

srl295 requested a review from miloush October 10, 2023 17:11

CLDR-17113 kbd: no nested gestures

490827f

miloush reviewed Oct 11, 2023

View reviewed changes

CLDR-17113 kbd: no nested gestures,2

846982c

miloush approved these changes Oct 11, 2023

View reviewed changes

srl295 merged commit 5f1d298 into main Oct 11, 2023

srl295 deleted the srl295/kbd/cldr-17113-naming branch October 11, 2023 21:02

srl295 mentioned this pull request Oct 11, 2023

CLDR-15034 kbd: cherry picks from main #3334

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLDR-17113 kbd: element/attr renaming #3313

CLDR-17113 kbd: element/attr renaming #3313

srl295 commented Oct 3, 2023

srl295 commented Oct 3, 2023

jira-pull-request-webhook bot commented Oct 3, 2023

mcdurdin left a comment

mcdurdin Oct 4, 2023

miloush Oct 5, 2023

mcdurdin Oct 6, 2023

miloush Oct 6, 2023

mcdurdin Oct 9, 2023

srl295 Oct 9, 2023

jira-pull-request-webhook bot commented Oct 4, 2023

jira-pull-request-webhook bot commented Oct 9, 2023

jira-pull-request-webhook bot commented Oct 9, 2023

jira-pull-request-webhook bot commented Oct 9, 2023

srl295 commented Oct 10, 2023

miloush commented Oct 10, 2023

mcdurdin commented Oct 10, 2023

miloush commented Oct 10, 2023

srl295 commented Oct 10, 2023 via email

srl295 commented Oct 10, 2023

miloush commented Oct 10, 2023

srl295 commented Oct 10, 2023

srl295 commented Oct 10, 2023

srl295 commented Oct 10, 2023

miloush commented Oct 10, 2023

srl295 commented Oct 10, 2023

mcdurdin commented Oct 10, 2023

srl295 commented Oct 11, 2023

miloush Oct 11, 2023

srl295 Oct 11, 2023

srl295 Oct 11, 2023

miloush commented Oct 11, 2023

CLDR-17113 kbd: element/attr renaming #3313

CLDR-17113 kbd: element/attr renaming #3313

Conversation

srl295 commented Oct 3, 2023

srl295 commented Oct 3, 2023

jira-pull-request-webhook bot commented Oct 3, 2023

mcdurdin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jira-pull-request-webhook bot commented Oct 4, 2023

jira-pull-request-webhook bot commented Oct 9, 2023

jira-pull-request-webhook bot commented Oct 9, 2023

jira-pull-request-webhook bot commented Oct 9, 2023

srl295 commented Oct 10, 2023

miloush commented Oct 10, 2023

mcdurdin commented Oct 10, 2023

miloush commented Oct 10, 2023

srl295 commented Oct 10, 2023 via email

srl295 commented Oct 10, 2023

miloush commented Oct 10, 2023

srl295 commented Oct 10, 2023

srl295 commented Oct 10, 2023

srl295 commented Oct 10, 2023

miloush commented Oct 10, 2023

srl295 commented Oct 10, 2023

mcdurdin commented Oct 10, 2023

srl295 commented Oct 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miloush commented Oct 11, 2023