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(core,developer): 'other' in modifiers 🙀 #11072

Closed
srl295 opened this issue Mar 25, 2024 · 13 comments · Fixed by #11118
Closed

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

srl295 opened this issue Mar 25, 2024 · 13 comments · Fixed by #11118
Assignees
Labels
Milestone

Comments

@srl295
Copy link
Member

srl295 commented Mar 25, 2024

Spec compliance: modifiers="other" is not supported.

Could we add a bitvalue 0x10000000 (to not collide with kmx_file.h) for the 'other' fallback?

(Edit it is other not default)

@srl295 srl295 added this to the B17S4 milestone Mar 25, 2024
@srl295 srl295 self-assigned this Mar 25, 2024
@mcdurdin
Copy link
Member

I need some more context on this. How is modifiers=default not bitmask 0x00000000?

(Also not quite seeing how this relates to #11040?)

@srl295
Copy link
Member Author

srl295 commented Mar 27, 2024

I need some more context on this. How is modifiers=default not bitmask 0x00000000?

This will be an additional entry in the kmap, that is, if an explicit match isn't found, try again using modifiers=X (where X is 0x10000000 as proposed).

Bitmask 0x00000000 is for no modifiers which may be a different layer.

<layer modifiers="none"> ...</layer>
<layer modifiers="default"> ...</layer>

(Also not quite seeing how this relates to #11040?)

That issue refactored the layers, but that's not relevant to the issue, only to the implementation. Removed the reference.

@mcdurdin
Copy link
Member

The bitflags in question are these, right?

#define LCTRLFLAG 0x0001 // Left Control flag
#define RCTRLFLAG 0x0002 // Right Control flag
#define LALTFLAG 0x0004 // Left Alt flag
#define RALTFLAG 0x0008 // Right Alt flag
#define K_SHIFTFLAG 0x0010 // Either shift flag
#define K_CTRLFLAG 0x0020 // Either ctrl flag
#define K_ALTFLAG 0x0040 // Either alt flag
//#define K_METAFLAG 0x0080 // Either Meta-key flag (tentative). Not usable in keyboard rules;
// Used internally (currently, only by KMW) to ensure Meta-key
// shortcuts safely bypass rules
// Meta key = Command key on macOS, Windows key on Windows
#define CAPITALFLAG 0x0100 // Caps lock on
#define NOTCAPITALFLAG 0x0200 // Caps lock NOT on
#define NUMLOCKFLAG 0x0400 // Num lock on
#define NOTNUMLOCKFLAG 0x0800 // Num lock NOT on
#define SCROLLFLAG 0x1000 // Scroll lock on
#define NOTSCROLLFLAG 0x2000 // Scroll lock NOT on
#define ISVIRTUALKEY 0x4000 // It is a Virtual Key Sequence
#define VIRTUALCHARKEY 0x8000 // Keyman 6.0: Virtual Key Cap Sequence NOT YET
#define K_MODIFIERFLAG 0x007F
#define K_NOTMODIFIERFLAG 0xFF00 // I4548

Those are 16 bit only, so we can start at 0x10000 for new flags in the KMXPlus code. Note that legacy .kmx only supports 16-bit because vk+modifiers are wedged into a single 32-bit value, but I don't think that's true for KMXPlus. We would need to make sure we add documentary evidence to kmx_file.h and other places where these bitflags are defined.

codes.ts

modifierCodes: {
"LCTRL":0x0001, // LCTRLFLAG
"RCTRL":0x0002, // RCTRLFLAG
"LALT":0x0004, // LALTFLAG
"RALT":0x0008, // RALTFLAG
"SHIFT":0x0010, // K_SHIFTFLAG
"CTRL":0x0020, // K_CTRLFLAG
"ALT":0x0040, // K_ALTFLAG
// TENTATIVE: Represents command keys, which some OSes use for shortcuts we don't
// want to block. No rule will ever target a modifier set with this bit set to 1.
"META":0x0080, // K_METAFLAG
"CAPS":0x0100, // CAPITALFLAG
"NO_CAPS":0x0200, // NOTCAPITALFLAG
"NUM_LOCK":0x0400, // NUMLOCKFLAG
"NO_NUM_LOCK":0x0800, // NOTNUMLOCKFLAG
"SCROLL_LOCK":0x1000, // SCROLLFLAG
"NO_SCROLL_LOCK":0x2000, // NOTSCROLLFLAG
"VIRTUAL_KEY":0x4000, // ISVIRTUALKEY
"VIRTUAL_CHAR_KEY":0x8000 // VIRTUALCHARKEY // Unused by KMW, but reserved for use by other Keyman engines.
},
modifierBitmasks: {
"ALL":0x007F,
"ALT_GR_SIM": (0x0001 | 0x0004),
"CHIRAL":0x001F, // The base bitmask for chiral keyboards. Includes SHIFT, which is non-chiral.
"IS_CHIRAL":0x000F, // Used to test if a bitmask uses a chiral modifier.
"NON_CHIRAL":0x0070, // The default bitmask, for non-chiral keyboards,
// Represents all modifier codes not supported by KMW 1.0 legacy keyboards.
"NON_LEGACY": 0x006F // ALL, but without the SHIFT bit
},
stateBitmasks: {
"ALL":0x3F00,
"CAPS":0x0300,
"NUM_LOCK":0x0C00,
"SCROLL_LOCK":0x3000
},

kmx.ts

(not unified with @keymanapp/common-types codes.ts due to history and dependency challenges, plan to unify in 18.0)

public static readonly LCTRLFLAG = 0x0001; // Left Control flag
public static readonly RCTRLFLAG = 0x0002; // Right Control flag
public static readonly LALTFLAG = 0x0004; // Left Alt flag
public static readonly RALTFLAG = 0x0008; // Right Alt flag
public static readonly K_SHIFTFLAG = 0x0010; // Either shift flag
public static readonly K_CTRLFLAG = 0x0020; // Either ctrl flag
public static readonly K_ALTFLAG = 0x0040; // Either alt flag
//public static readonly K_METAFLAG = 0x0080; // Either Meta-key flag (tentative). Not usable in keyboard rules;
// Used internally (currently, only by KMW) to ensure Meta-key
// shortcuts safely bypass rules
// Meta key = Command key on macOS, Windows key on Windows
public static readonly CAPITALFLAG = 0x0100; // Caps lock on
public static readonly NOTCAPITALFLAG = 0x0200; // Caps lock NOT on
public static readonly NUMLOCKFLAG = 0x0400; // Num lock on
public static readonly NOTNUMLOCKFLAG = 0x0800; // Num lock NOT on
public static readonly SCROLLFLAG = 0x1000; // Scroll lock on
public static readonly NOTSCROLLFLAG = 0x2000; // Scroll lock NOT on
public static readonly ISVIRTUALKEY = 0x4000; // It is a Virtual Key Sequence
public static readonly VIRTUALCHARKEY = 0x8000; // Keyman 6.0: Virtual Key Cap Sequence NOT YET
public static readonly MASK_MODIFIER_CHIRAL = KMXFile.LCTRLFLAG | KMXFile.RCTRLFLAG | KMXFile.LALTFLAG | KMXFile.RALTFLAG;
public static readonly MASK_MODIFIER_SHIFT = KMXFile.K_SHIFTFLAG;
public static readonly MASK_MODIFIER_NONCHIRAL = KMXFile.K_CTRLFLAG | KMXFile.K_ALTFLAG;
public static readonly MASK_STATEKEY = KMXFile.CAPITALFLAG | KMXFile.NOTCAPITALFLAG |
KMXFile.NUMLOCKFLAG | KMXFile.NOTNUMLOCKFLAG |
KMXFile.SCROLLFLAG | KMXFile.NOTSCROLLFLAG;
public static readonly MASK_KEYTYPE = KMXFile.ISVIRTUALKEY | KMXFile.VIRTUALCHARKEY;
public static readonly MASK_MODIFIER = KMXFile.MASK_MODIFIER_CHIRAL | KMXFile.MASK_MODIFIER_SHIFT | KMXFile.MASK_MODIFIER_NONCHIRAL;
public static readonly MASK_KEYS = KMXFile.MASK_MODIFIER | KMXFile.MASK_STATEKEY;
public static readonly KMX_MASK_VALID = KMXFile.MASK_KEYS | KMXFile.MASK_KEYTYPE;
public static readonly K_MODIFIERFLAG = 0x007F;
public static readonly K_NOTMODIFIERFLAG = 0xFF00; // I4548

legacy_kmx_file.h

Another one we want to unify but tricky, hoping to drop legacy_kmx_file.h in 18.0.

#define LCTRLFLAG 0x0001 // Left Control flag
#define RCTRLFLAG 0x0002 // Right Control flag
#define LALTFLAG 0x0004 // Left Alt flag
#define RALTFLAG 0x0008 // Right Alt flag
#define K_SHIFTFLAG 0x0010 // Either shift flag
#define K_CTRLFLAG 0x0020 // Either ctrl flag
#define K_ALTFLAG 0x0040 // Either alt flag
//#define K_METAFLAG 0x0080 // Either Meta-key flag (tentative). Not usable in keyboard rules;
// Used internally (currently, only by KMW) to ensure Meta-key
// shortcuts safely bypass rules
// Meta key = Command key on macOS, Windows key on Windows
#define CAPITALFLAG 0x0100 // Caps lock on
#define NOTCAPITALFLAG 0x0200 // Caps lock NOT on
#define NUMLOCKFLAG 0x0400 // Num lock on
#define NOTNUMLOCKFLAG 0x0800 // Num lock NOT on
#define SCROLLFLAG 0x1000 // Scroll lock on
#define NOTSCROLLFLAG 0x2000 // Scroll lock NOT on
#define ISVIRTUALKEY 0x4000 // It is a Virtual Key Sequence
#define VIRTUALCHARKEY 0x8000 // Keyman 6.0: Virtual Key Cap Sequence NOT YET
#define K_MODIFIERFLAG 0x007F
#define K_NOTMODIFIERFLAG 0xFF00 // I4548

kmxfileconsts.pas

Also hopefully disappearing in 18.0.

const
KMX_LCTRLFLAG = $0001; // Left Control flag
KMX_RCTRLFLAG = $0002; // Right Control flag
KMX_LALTFLAG = $0004; // Left Alt flag
KMX_RALTFLAG = $0008; // Right Alt flag
KMX_SHIFTFLAG = $0010; // Either shift flag
KMX_CTRLFLAG = $0020; // Either ctrl flag
KMX_ALTFLAG = $0040; // Either alt flag
KMX_CAPITALFLAG = $0100; // Caps lock on
KMX_NOTCAPITALFLAG = $0200; // Caps lock NOT on
KMX_NUMLOCKFLAG = $0400; // Num lock on
KMX_NOTNUMLOCKFLAG = $0800; // Num lock NOT on
KMX_SCROLLFLAG = $1000; // Scroll lock on
KMX_NOTSCROLLFLAG = $2000; // Scroll lock NOT on
KMX_ISVIRTUALKEY = $4000; // It is a Virtual Key Sequence
KMX_VIRTUALCHARKEY = $8000; // It is a virtual character key sequence - mnemonic layouts
// Combinations of key masks
KMX_MASK_MODIFIER_CHIRAL = KMX_LCTRLFLAG or KMX_RCTRLFLAG or KMX_LALTFLAG or KMX_RALTFLAG;
KMX_MASK_MODIFIER_SHIFT = KMX_SHIFTFLAG;
KMX_MASK_MODIFIER_NONCHIRAL = KMX_CTRLFLAG or KMX_ALTFLAG;
KMX_MASK_STATEKEY = KMX_CAPITALFLAG or KMX_NOTCAPITALFLAG or
KMX_NUMLOCKFLAG or KMX_NOTNUMLOCKFLAG or
KMX_SCROLLFLAG or KMX_NOTSCROLLFLAG;
KMX_MASK_KEYTYPE = KMX_ISVIRTUALKEY or KMX_VIRTUALCHARKEY;
KMX_MASK_MODIFIER = KMX_MASK_MODIFIER_CHIRAL or KMX_MASK_MODIFIER_SHIFT or KMX_MASK_MODIFIER_NONCHIRAL;
KMX_MASK_KEYS = KMX_MASK_MODIFIER or KMX_MASK_STATEKEY;
KMX_MASK_VALID = KMX_MASK_KEYS or KMX_MASK_KEYTYPE;

C7043_ldml.md

- `mod`: 32-bit bitfield defined as below. Little endian values.
| Value | Meaning |`kmx_file.h` | Comment |
|----------|----------|---------------|---------------------------------------------|
| 0x0000 | `none` | | All zeros = no modifiers |
| 0x0001 | `ctrlL` | `LCTRLFLAG` | Left Control |
| 0x0002 | `ctrlR` | `RCTRLFLAG` | Right Control |
| 0x0004 | `altL` | `LALTFLAG` | Left Alt |
| 0x0008 | `altR` | `RALTFLAG` | Right Alt |
| 0x0010 | `shift` | `K_SHIFTFLAG` | Either Shift |
| 0x0020 | `ctrl` | `K_CTRLFLAG` | Either Control |
| 0x0040 | `alt` | `K_ALTFLAG` | Either Alt |
| 0x0100 | `caps` | `CAPITALFLAG` | Caps lock |

KMBinaryFileFormat.h

Keyman Engine for Mac still uses this for loading kmx metadata

// ShiftFlags
#define LCTRLFLAG 0x0001 // Left Control flag
#define RCTRLFLAG 0x0002 // Right Control flag
#define LALTFLAG 0x0004 // Left Alt flag
#define RALTFLAG 0x0008 // Right Alt flag
#define K_SHIFTFLAG 0x0010 // Either shift flag
#define K_CTRLFLAG 0x0020 // Either ctrl flag
#define K_ALTFLAG 0x0040 // Either alt flag
//#define K_METAFLAG 0x0080 // Either Meta-key flag (tentative). Not usable in keyboard rules;
// Used internally (currently, only by KMW) to ensure Meta-key
// shortcuts safely bypass rules
// Meta key = Command key on macOS, Windows key on Windows
#define CAPITALFLAG 0x0100 // Caps lock on
#define NOTCAPITALFLAG 0x0200 // Caps lock NOT on
#define NUMLOCKFLAG 0x0400 // Num lock on
#define NOTNUMLOCKFLAG 0x0800 // Num lock NOT on
#define SCROLLFLAG 0x1000 // Scroll lock on
#define NOTSCROLLFLAG 0x2000 // Scroll lock NOT on
#define ISVIRTUALKEY 0x4000 // It is a Virtual Key Sequence
#define VIRTUALCHARKEY 0x8000 // Keyman 6.0: Virtual Key Cap Sequence
#define K_MODIFIERFLAG 0x007F
#define K_CAPITALMASK (CAPITALFLAG|NOTCAPITALFLAG)

etl2log load.cpp

Should be able to use kmx_file.h, really...

#define LCTRLFLAG 0x0001 // Left Control flag
#define RCTRLFLAG 0x0002 // Right Control flag
#define LALTFLAG 0x0004 // Left Alt flag
#define RALTFLAG 0x0008 // Right Alt flag
#define K_SHIFTFLAG 0x0010 // Either shift flag
#define K_CTRLFLAG 0x0020 // Either ctrl flag
#define K_ALTFLAG 0x0040 // Either alt flag
#define CAPITALFLAG 0x0100 // Caps lock on
#define NOTCAPITALFLAG 0x0200 // Caps lock NOT on
#define NUMLOCKFLAG 0x0400 // Num lock on
#define NOTNUMLOCKFLAG 0x0800 // Num lock NOT on
#define SCROLLFLAG 0x1000 // Scroll lock on
#define NOTSCROLLFLAG 0x2000 // Scroll lock NOT on
#define ISVIRTUALKEY 0x4000 // It is a Virtual Key Sequence
#define VIRTUALCHARKEY 0x8000 // Keyman 6.0: Virtual Key Cap Sequence NOT YET

Related issues

@srl295
Copy link
Member Author

srl295 commented Mar 28, 2024

OK, i'll start with 0x10000 then.

srl295 added a commit that referenced this issue Mar 28, 2024
- add a new value, 0x10000 to indicate 'default'

For: #11072
srl295 added a commit that referenced this issue Mar 28, 2024
@srl295 srl295 linked a pull request Mar 28, 2024 that will close this issue
srl295 added a commit that referenced this issue Mar 29, 2024
- remove default flag from kmx_file.h

Fixes: #11072
srl295 added a commit that referenced this issue Mar 29, 2024
- remove default flag from kmx_file.h

Fixes: #11072
srl295 added a commit that referenced this issue Mar 29, 2024
@mcdurdin mcdurdin modified the milestones: B17S4, B17S5 Mar 30, 2024
srl295 added a commit that referenced this issue Apr 1, 2024
- at least add comments pointing back to keyman_core_ldml.ts

Fixes: #11072
@jahorton
Copy link
Contributor

jahorton commented Apr 2, 2024

I need some more context on this. How is modifiers=default not bitmask 0x00000000?

This will be an additional entry in the kmap, that is, if an explicit match isn't found, try again using modifiers=X (where X is 0x10000000 as proposed).

Bitmask 0x00000000 is for no modifiers which may be a different layer.

<layer modifiers="none"> ...</layer>
<layer modifiers="default"> ...</layer>

I feel like I've missed something here. We're saying that "default" modifiers may be different from no ("none") modifiers?

The closest thing I could find in the spec: https://github.com/srl295/cldr/blob/master/docs/ldml/tr35-keyboards.md#57-element-settings. I see no such reference in either https://github.com/srl295/cldr/blob/master/docs/ldml/tr35-keyboards.md#58-element-keymap or https://github.com/srl295/cldr/blob/master/docs/ldml/tr35-keyboards.md#513-element-layer at this time (as "default" is not listed as valid for either).

But, fallback="omit" is a global, top-level setting, clearly for fallback behavior. At that point, since we failed to match, why could we not just hard-set the modifier bits to 0x00000 and try again instead of using a unique bitmap here? Or, have I missed recent changes to the spec that would clarify your intent here?

@srl295
Copy link
Member Author

srl295 commented Apr 2, 2024

@jahorton srl295/master is 2years out of date. Current proposed draft is:

https://www.unicode.org/reports/tr35/proposed.html - click through to Part 7.

I feel like I've missed something here. We're saying that "default" modifiers may be different from no ("none") modifiers?

Yes. See example above.

Fallback was removed.

@mcdurdin
Copy link
Member

mcdurdin commented Apr 2, 2024

Note: the current draft says 'other' rather than 'default'

There is one special case: the other layer matches if and only if no other layer matches. Thus logically the other layer is matched after all other layers have been checked.

@jahorton
Copy link
Contributor

jahorton commented Apr 2, 2024

@jahorton srl295/master is 2years out of date. Current proposed draft is:

https://www.unicode.org/reports/tr35/proposed.html - click through to Part 7.

I feel like I've missed something here. We're saying that "default" modifiers may be different from no ("none") modifiers?

Yes. See example above.

Fallback was removed.

... wow, how did I miss that it was srl295/master and not unicode's master? 🤦 My apologies there.

For future ease of reference: https://cldr-smoke.unicode.org/spec/main/ldml/tr35-keyboards.html#layer-modifier-matching

That section clears up the reasoning; thanks. Just had to find the right reference.

@srl295
Copy link
Member Author

srl295 commented Apr 2, 2024

Note: the current draft says 'other' rather than 'default'

you are right! Okay- will fix.

There is one special case: the other layer matches if and only if no other layer matches. Thus logically the other layer is matched after all other layers have been checked.

@srl295
Copy link
Member Author

srl295 commented Apr 2, 2024

@jahorton srl295/master is 2years out of date. Current proposed draft is:

https://www.unicode.org/reports/tr35/proposed.html - click through to Part 7.

I feel like I've missed something here. We're saying that "default" modifiers may be different from no ("none") modifiers?

Yes. See example above.

Fallback was removed.

... wow, how did I miss that it was srl295/master and not unicode's master? 🤦 My apologies there.

For future ease of reference: https://cldr-smoke.unicode.org/spec/main/ldml/tr35-keyboards.html#layer-modifier-matching

Proposed.html is a stable url. The above is a staging server.

That section clears up the reasoning; thanks. Just had to find the right reference.

Welcome

@jahorton
Copy link
Contributor

jahorton commented Apr 2, 2024

For future ease of reference: https://cldr-smoke.unicode.org/spec/main/ldml/tr35-keyboards.html#layer-modifier-matching

Proposed.html is a stable url. The above is a staging server.

It's the link I reached from the URL you gave. Is a "stable" version of the precise, drilled-down link possible? The URL forwarding scheme used for stable links isn't immediately clear to me.

Since the spec is so huge, it really helps to have direct links for discussion like this, hence why I included it in my comment.

@srl295
Copy link
Member Author

srl295 commented Apr 2, 2024

For future ease of reference: https://cldr-smoke.unicode.org/spec/main/ldml/tr35-keyboards.html#layer-modifier-matching

Proposed.html is a stable url. The above is a staging server.

It's the link I reached from the URL you gave. Is a "stable" version of the precise, drilled-down link possible? The URL forwarding scheme used for stable links isn't immediately clear to me.

Since the spec is so huge, it really helps to have direct links for discussion like this, hence why I included it in my comment.

Totally understandable. I was just trying to give some caution. No there isn't a stable link to the latest drilled-down subsection. It's a gap.

srl295 added a commit that referenced this issue Apr 2, 2024
- the 'other' keyword was incorrectly called 'default'

Fixes: #11072
@srl295 srl295 changed the title feat(core,developer): 'default' in modifiers 🙀 feat(core,developer): 'other' in modifiers 🙀 Apr 2, 2024
@srl295
Copy link
Member Author

srl295 commented Apr 2, 2024

fixed

@srl295 srl295 closed this as completed Apr 2, 2024
srl295 added a commit that referenced this issue Apr 4, 2024
- add a new value, 0x10000 to indicate 'default'

For: #11072
srl295 added a commit that referenced this issue Apr 4, 2024
srl295 added a commit that referenced this issue Apr 4, 2024
- remove default flag from kmx_file.h

Fixes: #11072
mcdurdin pushed a commit that referenced this issue Jun 25, 2024
- add a new value, 0x10000 to indicate 'default'

For: #11072
mcdurdin pushed a commit that referenced this issue Jun 25, 2024
mcdurdin pushed a commit that referenced this issue Jun 25, 2024
- remove default flag from kmx_file.h

Fixes: #11072
mcdurdin pushed a commit that referenced this issue Jun 25, 2024
mcdurdin pushed a commit that referenced this issue Jun 25, 2024
- at least add comments pointing back to keyman_core_ldml.ts

Fixes: #11072
mcdurdin pushed a commit that referenced this issue Jun 25, 2024
- the 'other' keyword was incorrectly called 'default'

Fixes: #11072
mcdurdin added a commit that referenced this issue Aug 24, 2024
While the modifier state property in core's API is 16-bit, internally
ldml_processor supports the modifier flag LDML_KEYS_MOD_OTHER with a
value of `0x10000`, which requires widening the value (we match the
32-bit size of the KMX_DWORD value from KMX+).

Note: this is not yet well unit-tested.

Relates-to: #11072
Fixes: #12057
mcdurdin added a commit that referenced this issue Aug 24, 2024
While the modifier state property in core's API is 16-bit, internally
ldml_processor supports the modifier flag LDML_KEYS_MOD_OTHER with a
value of `0x10000`, which requires widening the value (we match the
32-bit size of the KMX_DWORD value from KMX+).

Note: this is not yet well unit-tested.

Relates-to: #11072
Fixes: #12057
mcdurdin added a commit that referenced this issue Aug 26, 2024
While the modifier state property in core's API is 16-bit, internally
ldml_processor supports the modifier flag LDML_KEYS_MOD_OTHER with a
value of `0x10000`, which requires widening the value (we match the
32-bit size of the KMX_DWORD value from KMX+).

Note: this is not yet well unit-tested.

Relates-to: #11072
Fixes: #12057
Cherry-pick-of: #12281
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants