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)

[0HyperCube]

Thanks again for the review @TrueDoctor. I've tested your changes by sampling some of the changed and unchanged bindings. The functionality seems to work well and I didn't spot any regressions.

[Keavon]

This will require my approval before the merge because of the concern I raised but do not have time presently to confirm. Marking as Request Changes to block its merge until I get the time.

[timon-schelling]

LGTM

[0HyperCube]

I have this planned to be something I look into after the 1.0 release.

You'll be busy doing other things after the release like promoting, talking to new people, improving guides and documentation, dealing with an expanded pull request count from new contributors, etc.

I cannot justify doing it before then given competing priorities.

That is fine. TrueDoctor and Timon have both reviewed it and they are very experienced and capable of testing functionality (this PR has no controversial UX changes).

However I believe it most likely is not something we should land unless it does solve the problem

It does solve a bug where the tab whilst dragging a handle in the path tool would both switch to another handle and change to the select tool. It also removes the unintuitive behaviour that allowed this to happen in the first place (both a KeyDown and a KeyDownNoRepeat event firing at the same time). This was clearly explained in the PR description.

without the regressions I encountered when trying this myself.

As previously mentioned, I am not able to read your mind and determine what regressions to look for. You are seemingly unable to give any further information about the previous approach. However this PR has been extensively tested by myself and TrueDoctor and no issues have been found.

I find it a bit baffling that you merge thousand line changes without review but when it comes to this rather simple change (the line count only inflated by the changes in the mappings from KeyDown to KeyDownNoRepeat) you require extensive review from all of the core team members?

[TrueDoctor]

Delaying this past 1.0 is totally unreasonable, I manually tested the changes to confirm they resolve the issue as it was originally described. We don't strive for perfection in the code we merge, especially if it delivers tangible UX improvements in the short term. Following the precedent set in #4335 I am very tempted to just merge this as is since the changes are very isolated and the code is actually reviewed. You are free to make a followup pr to fix any regressions introduced by this

[0HyperCube]

Hi @TrueDoctor. It seems this PR is merged but then has been (accidentally?) deleted/force pushed from the commit history. Could you please clarify your intentions with this?

Thanks again for your time on this.

[Keavon]

Hi @0HyperCube, sorry for the trouble on your end, but it'll be necessary to open a second PR for this since GitHub does not allow reopening PRs post-merge.