keymanapp / keymanapp/keyman

question(developer): rule precedence is impacted by `if()` statements ... should it be?

Open
#6,851 0 comments 0 reactions 0 assignees View on GitHub
developer/ developer/compilers/ question
Dominant language
Pascal
Stars
534
Forks
143
Avg merge
2d 10h
Merged PRs (30d)
113

Description

**Describe the bug**

Trap for Keyman keyboard devs. `if` statements impact the rule precedence, as each `if` statement is treated as an element in the context, and [longer rules take precedence over shorter ones](https://help.keyman.com/developer/language/guide/rules#rule-order). For rules of same length, order is as written. This is per the design of the keyboard language, where a longer context match takes precedence over a shorter one, and totally makes sense for context matches. But it doesn't really make much sense for logical tests with `if`, `platform` and `baselayout` statements. Is this behaviour that we should adjust?

## Example

As it stands, in the code below, the compiled order of the rules is 1, 3, 2, because rule 3 has 2 elements in the 'context' whereas rule 2 has only 1 element in 'context':
```
group(PostKeystroke) readonly
c We get here after every keystroke and model action is processed

c RULE 1: Okay, let's stay on the numeric layer if we are there already
if(&newLayer = "") if(&layer = 'numeric') any(digit) > context

c RULE 2: Don't swap off the caps lock layer automatically
if(&layer = 'caps') > context

c RULE 3: no other changes, so detect sentence or layer change, as long
c as the user hasn't attempted to change layer themselves.
platform('touch') if(&newLayer = "") > use(detectStartOfSentence)
```

## Workaround

A workaround is to make sure that shorter rules have matching clauses, e.g. in this example, the compiled order is 1, 2, 3, because rules 2 and 3 have the same length 'context':

```
group(PostKeystroke) readonly
c We get here after every keystroke and model action is processed

c Okay, let's stay on the numeric layer if we are there already
platform('touch') if(&newLayer = "") if(&layer = 'numeric') any(digit) > context

c Don't swap off the caps lock layer automatically
platform('touch') if(&layer = 'caps') > context

c no other changes, so detect sentence or layer change, as long
c as the user hasn't attempted to change layer themselves.
platform('touch') if(&newLayer = "") > use(detectStartOfSentence)
```

## Discussion

This impacts all keyboards on all platforms. I chose the example above because it is purely `if` statements (`platform` and `baselayout` statements are compiled down to `if` statements [behind the scenes](https://github.com/keymanapp/keyman/blob/ec690f47f9979a9e8144dd3a9f1b06a4c820096f/developer/src/kmcmpdll/Compiler.cpp#L2618-L2664)).

This can be addressed entirely within the compiler. My proposal would be to use the same logic as we use for [`xstrlen_ignoreifopt`](https://github.com/keymanapp/keyman/blob/ec690f47f9979a9e8144dd3a9f1b06a4c820096f/common/windows/cpp/src/xstring.cpp#L131-L139) for determining context length. Sadly, doing this would introduce another compiler flag for backward compatibility for existing keyboards, so this needs careful consideration before implementation.

Group-level option:
```
group(mygroup) readonly rule-order(default) c matches Keyman 15 behaviour, default
group(mygroup) readonly rule-order(source) c do not re-sort the rules, use as given in file
group(mygroup) readonly rule-order(char-length) c sort by length of chars + dks in context, i.e. excluding `if()`
```
File-level option:
```
store(&rule-order) 'default' c ...
```

Note, if using source rule order, we could test in the compiler to see if certain rules are never fired due to earlier rules having shorter context (and matching `if` clauses)?

---

**Keyman Developer:**
- Keyman Developer version: 15.0.263-stable

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.