Update the input mapping system to not dispatch both a matched KeyDown and KeyDownNoRepeat

[Keavon]

For example, if we update input_mapping.rs like so:

- entry!(KeyDown(KeyA); action_dispatch=ToolMessage::ActivateToolPath),
+ entry!(KeyDownNoRepeat(KeyA); action_dispatch=ToolMessage::ActivateToolPath),

Then CtrlA will fire both the selection of all layers...

entry!(KeyDown(KeyA); modifiers=[Accel], action_dispatch=DocumentMessage::SelectAllLayers),

...as well as the change to the Path tool, since one gets matched on KeyDown and another on KeyDownNoRepeat. If both were KeyDown or KeyDownNoRepeat, only one would match. But currently, both are matching, and we only want one to match.

After implementing this fix, the following mappings should be changed from KeyDown to KeyDownNoRepeat:

Mappings to change

// TransformLayerMessage
entry!(KeyDown(KeyX); action_dispatch=TransformLayerMessage::ConstrainX),
entry!(KeyDown(KeyY); action_dispatch=TransformLayerMessage::ConstrainY),

// PathToolMessage
entry!(KeyDown(KeyG); action_dispatch=PathToolMessage::GRS { key: KeyG }),
entry!(KeyDown(KeyR); action_dispatch=PathToolMessage::GRS { key: KeyR }),
entry!(KeyDown(KeyS); action_dispatch=PathToolMessage::GRS { key: KeyS }),

// PenToolMessage
entry!(KeyDown(KeyG); action_dispatch=PenToolMessage::GRS { grab: KeyG, rotate: KeyR, scale: KeyS }),
entry!(KeyDown(KeyR); action_dispatch=PenToolMessage::GRS { grab: KeyG, rotate: KeyR, scale: KeyS }),
entry!(KeyDown(KeyS); action_dispatch=PenToolMessage::GRS { grab: KeyG, rotate: KeyR, scale: KeyS }),

// ToolMessage
entry!(KeyDown(KeyV); action_dispatch=ToolMessage::ActivateToolSelect),
entry!(KeyDown(KeyZ); action_dispatch=ToolMessage::ActivateToolNavigate),
entry!(KeyDown(KeyI); action_dispatch=ToolMessage::ActivateToolEyedropper),
entry!(KeyDown(KeyT); action_dispatch=ToolMessage::ActivateToolText),
entry!(KeyDown(KeyF); action_dispatch=ToolMessage::ActivateToolFill),
entry!(KeyDown(KeyH); action_dispatch=ToolMessage::ActivateToolGradient),
entry!(KeyDown(KeyA); action_dispatch=ToolMessage::ActivateToolPath),
entry!(KeyDown(KeyP); action_dispatch=ToolMessage::ActivateToolPen),
entry!(KeyDown(KeyN); action_dispatch=ToolMessage::ActivateToolFreehand),
entry!(KeyDown(KeyL); action_dispatch=ToolMessage::ActivateToolLine),
entry!(KeyDown(KeyM); action_dispatch=ToolMessage::ActivateToolRectangle),
entry!(KeyDown(KeyE); action_dispatch=ToolMessage::ActivateToolEllipse),
entry!(KeyDown(KeyY); action_dispatch=ToolMessage::ActivateToolPolygon),
entry!(KeyDown(KeyB); action_dispatch=ToolMessage::ActivateToolBrush),

// DocumentMessage
entry!(KeyDown(Space); modifiers=[Control], action_dispatch=DocumentMessage::GraphViewOverlayToggle),
entry!(KeyDown(KeyO); modifiers=[Alt], action_dispatch=DocumentMessage::ToggleOverlaysVisibility),
entry!(KeyDown(KeyS); modifiers=[Alt], action_dispatch=DocumentMessage::ToggleSnapping),
entry!(KeyDown(KeyL); modifiers=[Accel], action_dispatch=DocumentMessage::ToggleSelectedLocked),
entry!(KeyDown(KeyG); modifiers=[Alt], action_dispatch=DocumentMessage::ToggleGridVisibility),

// TransformLayerMessage
entry!(KeyDown(KeyG); action_dispatch=TransformLayerMessage::BeginGrab),
entry!(KeyDown(KeyR); action_dispatch=TransformLayerMessage::BeginRotate),
entry!(KeyDown(KeyS); action_dispatch=TransformLayerMessage::BeginScale),

// PortfolioMessage
entry!(KeyDown(KeyO); modifiers=[Accel], action_dispatch=PortfolioMessage::OpenDocument),
entry!(KeyDown(KeyI); modifiers=[Accel], action_dispatch=PortfolioMessage::Import),
entry!(KeyDown(KeyR); modifiers=[Alt], action_dispatch=PortfolioMessage::ToggleRulers),

// DialogMessage
entry!(KeyDown(KeyE); modifiers=[Accel], action_dispatch=DialogMessage::RequestExportDialog),
entry!(KeyDown(KeyN); modifiers=[Accel], action_dispatch=DialogMessage::RequestNewDocumentDialog),
entry!(KeyDown(Comma); modifiers=[Accel], action_dispatch=DialogMessage::RequestPreferencesDialog),

[tarunprabhu11]

Can I have a go at this?

[Keavon]

You're welcome to give it a try, sure. Just be aware that it is likely only something that can be successfully completed by someone with a good level of software engineering experience, since it's pretty tricky. I put a few hours into it and gave up. But if you're clever and have been programming for a while, you can probably figure it out better than I was able to.

[tarunprabhu11]

Hi @Keavon , just needed a few clarifications

• currently using crtl+a withentry!(KeyDownNoRepeat(KeyA); action_dispatch=ToolMessage::ActivateToolPath),will fire change to path tool and will not fire the selection of all layers. (can work on this)
• using ctrl+a with entry!(KeyDownNoRepeat(KeyA); modifiers=[Accel], action_dispatch=DocumentMessage::SelectAllLayers), will fire both actions.
  The other action remains KeyDown in both cases.
  Can I please get a recheck on the requirements.

[Keavon]

I don't understand what you've tested. Are you saying you changed KeyDown to KeyDownNoRepeat for both of your two makings you included in your comment, then attempted to use Ctrl+A? What is the outcome of that?

[tarunprabhu11]

When the change to path tool action is keydownnorepeat and the select all layers is keydown using ctrl+a would result in change to path tool only.
When the path tool is keydown and select all layers is keydownnorepeat using ctrl+a would result in both change to path tool and select all layers.

[tarunprabhu11]

Since it's a bit different from what's mentioned in the issue, I just wanted to reconfirm the desired working.

[Keavon]

It sounds like you're identifying examples of how this is broken. And yes, we know it's broken, so the goal here is to fix it. We want it to treat both as co-equals, so the existing manner in which a higher-precedence key supersedes a lower-precedence key works whether they are KeyDown or KeyDownNoRepeat or some mixture; instead of requiring they all be of the same type.

[tarunprabhu11]

Hello @Keavon, whenever you get a chance, I'd love your feedback on the PR. Please let me know if any changes are needed.

[0HyperCube]

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.

Perhaps I've missed something since apparently "it's pretty tricky"?