Improve no repeat priority

[0HyperCube]

Closes #2266

This seems like a rather trivial issue. Priority for mappings is determined by a) the number of modifiers then b) the order in declaration. When a key is initially pressed, it is necessary to match the highest priority action from both the repeat and non repeat list. This mandates that the lists are combined into one KeyMappingEntries for all key down events to preserve the ordering.

If the matched action is a KeyDownNoRepeat but the event is just a KeyDown then it is simply a noop. I do not think it would be useful to fall back in this case.

The fix benefits the path tool where it is now possible to use tab while dragging a handle to switch to the next handle again (previously it would change to the select tool).

I have not updated any of the input definitions as it seems it would be a bit subjective.

[cubic-dev-ai]

All reported issues were addressed across 6 files

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[TrueDoctor]

The code looks fine though @Keavon will have to decide if he is satisfied with the functionality

[TrueDoctor]

!build (Run ID 36229993284)

[Keavon]

I am fairly sure it is not this trivial, and something has been missed in the process. I will have to look into this in more detail to remind myself what that is. I don't recall from my current mental context because it has been at least a year since I touched this, but I do recall there being a trivial solution that broke something important. Sorry for the vague lack of details, I'll have more when I explore it, no promises when I can find the time but hopefully somewhat soon.

[github-actions]

📦 Web Build Complete for 06d3d1b
https://45d0e301.graphite.pages.dev

Wasm: 39.69 MB — JS: 0.46 MB — CSS: 0.09 MB — Fonts: 0.30 MB — Images: 0.09 MB — All Assets: 40.64 MB

[0HyperCube]

Thanks for the review @TrueDoctor.

With regards to @Keavon's comment:

something has been missed in the process

This solution appears to meet the requirements in the issue and I have tested to ensure that current shortcuts still function. You are welcome to do your own testing.

In fact they seemingly function better than before. For example the tab whilst dragging a handle with the path tool previously triggered both the change in handle as well as switching to the select tool. This is clearly not desirable and is similar to the example case stated in the issue.

I do recall there being a trivial solution that broke something important

It is perhaps possible that I have not implemented the exact same flawed solution? If you would be so kind as to link this previous solution then I would be able to compare it.

no promises when I can find the time

Might I kindly suggest that you could delegate the review of trivial bug fixes such as this to other maintainers since I feel they are also capable of evaluating the impact of the changes.

Thanks.

[TrueDoctor]

@Keavon unless you can find a counterexample or time to review the pr, I'll merge it in two days

[Keavon]

I have this planned to be something I look into after the 1.0 release. I cannot justify doing it before then given competing priorities. However I believe it most likely is not something we should land unless it does solve the problem without the regressions I encountered when trying this myself.

[TrueDoctor]

I confirmed the fix to work, @0HyperCube can you do a final verification that I didn't break anything before I merge this? (I also included a commit which changes the bindings mentioned in the original issue and confirmed they wors as expected)