feat(core): support modifiers=other🙀 #11118

srl295 · 2024-03-28T17:21:35Z

add a new value, 0x10000 to indicate 'other'
mention in kmx_file.h also
add default lookup to ldml_processor

For: #11072

@keymanapp-test-bot skip

- add a new value, 0x10000 to indicate 'default' For: #11072

keymanapp-test-bot · 2024-03-28T17:21:39Z

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

Android
Developer
iOS
- Keyman for iOS (simulator image)
- FirstVoices Keyboards for iOS (simulator image)
- TestFlight internal PR build version - 17.0.299 (0.11118.10666)
Keyboards
- Test Keyboards
macOS
- Keyman for macOS
Web
- KeymanWeb Test Home
Windows

…11072-default-modifier

Fixes: #11072

mcdurdin

We'll need to update all the other referenced files to add a comment about DEFAULT_MODIFIER to them as well

mcdurdin · 2024-03-29T04:43:19Z

common/include/kmx_file.h

@@ -302,6 +302,7 @@ namespace kmx {

 #define K_MODIFIERFLAG  0x007F
 #define K_NOTMODIFIERFLAG 0xFF00   // I4548
+#define K_DEFAULTMODFLAG  0x10000  // used by KMX+ for the default modifier


Given this cannot be used in this context, as modifier flags are 16 bit in .kmx, I think this should be a comment. Also, suggest using full word rather than MOD

Suggested change

#define K_DEFAULTMODFLAG 0x10000 // used by KMX+ for the default modifier

// Note: DEFAULT_MODIFIER = 0x10000, used by KMX+ for the

// default modifier flag in layers, > 16 bit so not available here

mcdurdin · 2024-03-29T04:43:48Z

core/src/ldml/C7043_ldml.md

+|  0x0020  | `ctrl`    | `K_CTRLFLAG`       | Either Control                                |
+|  0x0040  | `alt`     | `K_ALTFLAG`        | Either Alt                                    |
+|  0x0100  | `caps`    | `CAPITALFLAG`      | Caps lock                                     |
+|  0x10000 | `default` | `K_DEFAULTMODFLAG` | Default (not used in conjunction with others) |


per your other discussion it should be n/a here because it can't be used in kmx_file.h

mcdurdin · 2024-03-29T04:43:59Z

core/src/ldml/ldml_vkeys.cpp

+
+  // look for a layer with "default"
+  {
+    const vkey_id id_default(vk, (K_DEFAULTMODFLAG));


Suggested change

const vkey_id id_default(vk, (K_DEFAULTMODFLAG));

const vkey_id id_default(vk, (DEFAULT_MODIFIER));

that constant was removed, so using LDML_KEYS_MOD_DEFAULT instead

- remove default flag from kmx_file.h Fixes: #11072

srl295 · 2024-03-29T17:08:06Z

Sorry, accidental push from the wrong ticket.

- fix spec Fixes: #11072

mcdurdin

LGTM so far. But need comments similar to the one in kmx_file.h in the other headers mentioned in original issue: #11072 (comment)

srl295 · 2024-04-01T17:16:22Z

We'll need to update all the other referenced files to add a comment about DEFAULT_MODIFIER to them as well
...
LGTM so far. But need comments similar to the one in kmx_file.h in the other headers mentioned in original issue: #11072 (comment)

OK. The other referenced files by you and not the other files referenced by me. Got it!

Yes, I'll add comments, thanks.

- at least add comments pointing back to keyman_core_ldml.ts Fixes: #11072

srl295 · 2024-04-01T17:27:39Z

@mcdurdin PTAL. Pointing other usages back to keyman_core_ldml.ts

jahorton

I'm not privy to the inner workings of KMX+, but is there a reason 0x0000 can't act as DEFAULT_MODIFIER? Is it that you need some sort of target bit flag, rather than the absence of any?

mcdurdin · 2024-04-02T01:29:58Z

I'm not privy to the inner workings of KMX+, but is there a reason 0x0000 can't act as DEFAULT_MODIFIER? Is it that you need some sort of target bit flag, rather than the absence of any?

See the original issue #11072.

mcdurdin

LGTM, minor tweaks only

mcdurdin · 2024-04-02T02:03:26Z

common/web/keyboard-processor/src/text/codes.ts

-// There's an upcoming `/common/web/types` package that 'codes' and 'keyboards' may fit well within.
-// In fact, there's a file there (on its branch) that should be merged with this one!


There is a TODO for this. we haven't merged these yet.

right. was just trying to update it

common/windows/cpp/include/legacy_kmx_file.h

jahorton · 2024-04-02T02:10:10Z

I'm not privy to the inner workings of KMX+, but is there a reason 0x0000 can't act as DEFAULT_MODIFIER? Is it that you need some sort of target bit flag, rather than the absence of any?

See the original issue #11072.

I've done so, and have even cross-referenced the spec to try to gain clarity. Sadly, my question still remains - the issue didn't clarify anything for me. I've left a comment on the base issue about my concerns.

srl295 · 2024-04-02T02:32:13Z

Yes, no modifier is not default.

- the 'other' keyword was incorrectly called 'default' Fixes: #11072

srl295 · 2024-04-02T04:24:05Z

Updated to be other not default.

keyman-server · 2024-04-03T18:06:36Z

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

feat(core): kmx+ scaffolding for modifiers=default

313bbd3

- add a new value, 0x10000 to indicate 'default' For: #11072

srl295 self-assigned this Mar 28, 2024

github-actions bot added common/ core/ Keyman Core feat labels Mar 28, 2024

keymanapp-test-bot bot added this to the B17S4 milestone Mar 28, 2024

srl295 added 2 commits March 28, 2024 12:23

Merge branch 'feat/developer/11044-error-invalid-var' into feat/core/…

1691a59

…11072-default-modifier

fix(core): support default lookup for modifiers

89f5a0e

Fixes: #11072

srl295 marked this pull request as ready for review March 28, 2024 18:12

srl295 requested review from mcdurdin and rc-swag as code owners March 28, 2024 18:12

srl295 linked an issue Mar 28, 2024 that may be closed by this pull request

feat(core,developer): 'other' in modifiers 🙀 #11072

Closed

mcdurdin requested changes Mar 29, 2024

View reviewed changes

srl295 requested a review from mcdurdin March 29, 2024 16:58

chore(core): support default lookup for modifiers

bca037c

- remove default flag from kmx_file.h Fixes: #11072

srl295 force-pushed the feat/core/11072-default-modifier branch from b5c22e8 to bca037c Compare March 29, 2024 17:07

srl295 mentioned this pull request Mar 29, 2024

chore(core): dx better err message on embedded test vkeys 🙀 #11119

Merged

chore(core): support default lookup for modifiers

ae6f7b5

- fix spec Fixes: #11072

mcdurdin modified the milestones: B17S4, B17S5 Mar 30, 2024

mcdurdin requested changes Apr 1, 2024

View reviewed changes

Base automatically changed from feat/developer/11044-error-invalid-var to beta April 1, 2024 17:12

srl295 added 2 commits April 1, 2024 12:16

Merge branch 'beta' into feat/core/11072-default-modifier

d75bbbe

chore(common): add more references to 0x10000

5615a5e

- at least add comments pointing back to keyman_core_ldml.ts Fixes: #11072

srl295 requested a review from jahorton as a code owner April 1, 2024 17:26

srl295 requested a review from mcdurdin April 1, 2024 17:26

github-actions bot added the common/web/ label Apr 1, 2024

jahorton reviewed Apr 2, 2024

View reviewed changes

mcdurdin approved these changes Apr 2, 2024

View reviewed changes

fix(common,core): support other lookup for modifiers

5b1c853

- the 'other' keyword was incorrectly called 'default' Fixes: #11072

srl295 changed the title ~~feat(core): support modifiers=default 🙀~~ feat(core): support modifiers=other🙀 Apr 2, 2024

srl295 merged commit 9333dfa into beta Apr 2, 2024
24 of 25 checks passed

srl295 deleted the feat/core/11072-default-modifier branch April 2, 2024 21:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): support modifiers=other🙀 #11118

feat(core): support modifiers=other🙀 #11118

srl295 commented Mar 28, 2024 •

edited

Loading

keymanapp-test-bot bot commented Mar 28, 2024 •

edited

Loading

mcdurdin left a comment

mcdurdin Mar 29, 2024

mcdurdin Mar 29, 2024

srl295 Mar 29, 2024

mcdurdin Mar 29, 2024

srl295 Mar 29, 2024

srl295 commented Mar 29, 2024

mcdurdin left a comment •

edited

Loading

srl295 commented Apr 1, 2024

srl295 commented Apr 1, 2024 •

edited

Loading

jahorton left a comment

mcdurdin commented Apr 2, 2024

mcdurdin left a comment

mcdurdin Apr 2, 2024

srl295 Apr 2, 2024

jahorton commented Apr 2, 2024 •

edited

Loading

srl295 commented Apr 2, 2024

srl295 commented Apr 2, 2024

keyman-server commented Apr 3, 2024

	#define K_DEFAULTMODFLAG 0x10000 // used by KMX+ for the default modifier
	// Note: DEFAULT_MODIFIER = 0x10000, used by KMX+ for the
	// default modifier flag in layers, > 16 bit so not available here

	\| 0x10000 \| `default` \| `K_DEFAULTMODFLAG` \| Default (not used in conjunction with others) \|
	\| 0x10000 \| `default` \| `DEFAULT_MODIFIER` \| Default (not used in conjunction with others) \|

	const vkey_id id_default(vk, (K_DEFAULTMODFLAG));
	const vkey_id id_default(vk, (DEFAULT_MODIFIER));

		// There's an upcoming `/common/web/types` package that 'codes' and 'keyboards' may fit well within.
		// In fact, there's a file there (on its branch) that should be merged with this one!

feat(core): support modifiers=other🙀 #11118

feat(core): support modifiers=other🙀 #11118

Conversation

srl295 commented Mar 28, 2024 • edited Loading

keymanapp-test-bot bot commented Mar 28, 2024 • edited Loading

User Test Results

Test Artifacts

mcdurdin left a comment

Choose a reason for hiding this comment

mcdurdin Mar 29, 2024

Choose a reason for hiding this comment

mcdurdin Mar 29, 2024

Choose a reason for hiding this comment

srl295 Mar 29, 2024

Choose a reason for hiding this comment

mcdurdin Mar 29, 2024

Choose a reason for hiding this comment

srl295 Mar 29, 2024

Choose a reason for hiding this comment

srl295 commented Mar 29, 2024

mcdurdin left a comment • edited Loading

Choose a reason for hiding this comment

srl295 commented Apr 1, 2024

srl295 commented Apr 1, 2024 • edited Loading

jahorton left a comment

Choose a reason for hiding this comment

mcdurdin commented Apr 2, 2024

mcdurdin left a comment

Choose a reason for hiding this comment

mcdurdin Apr 2, 2024

Choose a reason for hiding this comment

srl295 Apr 2, 2024

Choose a reason for hiding this comment

jahorton commented Apr 2, 2024 • edited Loading

srl295 commented Apr 2, 2024

srl295 commented Apr 2, 2024

keyman-server commented Apr 3, 2024

srl295 commented Mar 28, 2024 •

edited

Loading

keymanapp-test-bot bot commented Mar 28, 2024 •

edited

Loading

mcdurdin left a comment •

edited

Loading

srl295 commented Apr 1, 2024 •

edited

Loading

jahorton commented Apr 2, 2024 •

edited

Loading