kicad-developers team mailing list archive

[Ian McInerney <Ian.S.McInerney@xxxxxxxx>]

I was just doing some digging into the processing of the shifted key codes
because there was another bug filed about them (
https://bugs.launchpad.net/kicad/+bug/1838420), and there seems to be
another problem with their processing.

Inside RunHotkey there are 2 searches performed for hotkeys, the first for
the actual key combination given and the second (if nothing is found in the
first) with the shift modifier removed. This is posing a problem with the
untranslated shifted key codes in the wxEVT_CHAR_HOOK, since if there is
any hotkey on the non-shifted key then it will be run and the event is
stopped processing and the corresponding wxEVT_CHAR is never generated.

For instance, when I set the hotkey for switch track posture to be * (which
on my keyboad is Shift+8) and also have tune differential pair length on
hotkey 8 then the wxEVT_CHAR_HOOK event runs the tune action and stops the
event processing, see the below trace:

11:32:28 AM: Trace: (KICAD_KEY_EVENTS) EDA_BASE_FRAME::OnCharHook    Hook
        SHIFT   306   --S-    0 (U+0000)    65505    0x00000032  (  298,
 171)
11:32:28 AM: Trace: (KICAD_KEY_EVENTS) TOOL_DISPATCHER::DispatchWxEvent
 Hook             '8'    56   --S-   56 (U+0038)       42    0x00000011  (
 298,  171)
11:32:28 AM: Trace: (KICAD_TOOL_STACK) TOOL_DISPATCHER::DispatchWxEvent
category: keyboard  action: key-pressed key: 56 mods: shift
11:32:28 AM: Trace: (KICAD_TOOL_STACK) TOOL_MANAGER::processEvent category:
keyboard  action: key-pressed key: 56 mods: shift
11:32:28 AM: Trace: (KICAD_TOOL_STACK) ACTION_MANAGER::RunHotKey Key:
Shift+8
11:32:28 AM: Trace: (KICAD_TOOL_STACK) ACTION_MANAGER::RunHotKey No actions
found, searching with key: 8
11:32:28 AM: Trace: (KICAD_TOOL_STACK) ACTION_MANAGER::RunHotKey Running
action: pcbnew.LengthTuner.TuneDiffPair for hotkey Shift+8
11:32:28 AM: Trace: (KICAD_TOOL_STACK) TOOL_MANAGER::processEvent category:
command  action: activate cmd-str: pcbnew.LengthTuner.TuneDiffPair
11:32:28 AM: Trace: (KICAD_TOOL_STACK) TOOL_MANAGER::dispatchInternal
category: command  action: activate cmd-str: pcbnew.LengthTuner.TuneDiffPair
11:32:28 AM: Trace: (KICAD_TOOL_STACK) TOOL_MANAGER::dispatchInternal
Waking tool pcbnew.InteractiveRouter for event: category: command  action:
activate cmd-str: pcbnew.LengthTuner.TuneDiffPair
11:32:28 AM: Trace: (KICAD_TOOL_STACK) TOOL_MANAGER::dispatchInternal
Waking tool pcbnew.InteractiveSelection for event: category: command
 action: activate cmd-str: pcbnew.LengthTuner.TuneDiffPair
11:32:28 AM: Trace: (KICAD_TOOL_STACK) TOOL_MANAGER::dispatchInternal
Running tool pcbnew.LengthTuner for event: category: command  action:
activate cmd-str: pcbnew.LengthTuner.TuneDiffPair
11:32:28 AM: Trace: (KICAD_TOOL_STACK) TOOL_MANAGER::processEvent category:
command  action: action cmd-str: pcbnew.InteractiveSelection.Clear
11:32:28 AM: Trace: (KICAD_TOOL_STACK) TOOL_MANAGER::dispatchInternal
category: command  action: action cmd-str: pcbnew.InteractiveSelection.Clear
11:32:28 AM: Trace: (KICAD_TOOL_STACK) TOOL_MANAGER::dispatchInternal
Waking tool pcbnew.InteractiveSelection for event: category: command
 action: action cmd-str: pcbnew.InteractiveSelection.Clear
11:32:28 AM: Trace: (KICAD_TOOL_STACK) TOOL_MANAGER::dispatchInternal
Running tool pcbnew.InteractiveSelection for event: category: command
 action: action cmd-str: pcbnew.InteractiveSelection.Clear
11:32:28 AM: Trace: (KICAD_TOOL_STACK) TOOL_MANAGER::dispatchInternal
handled: true  category: command  action: action cmd-str:
pcbnew.InteractiveSelection.Clear
11:32:28 AM: Trace: (KICAD_TOOL_STACK) TOOL_MANAGER::dispatchActivation
category: command  action: action cmd-str: pcbnew.InteractiveSelection.Clear
11:32:28 AM: Trace: (KICAD_TOOL_STACK) TOOL_MANAGER::processEvent handled:
true  category: command  action: action cmd-str:
pcbnew.InteractiveSelection.Clear
11:32:28 AM: Trace: (KICAD_TOOL_STACK) TOOL_MANAGER::processEvent category:
command  action: activate cmd-str: pcbnew.LengthTuner
11:32:28 AM: Trace: (KICAD_TOOL_STACK) TOOL_MANAGER::dispatchInternal
category: command  action: activate cmd-str: pcbnew.LengthTuner
11:32:28 AM: Trace: (KICAD_TOOL_STACK) TOOL_MANAGER::dispatchInternal
Waking tool pcbnew.InteractiveSelection for event: category: command
 action: activate cmd-str: pcbnew.LengthTuner
11:32:28 AM: Trace: (KICAD_TOOL_STACK) TOOL_MANAGER::dispatchInternal
handled: false  category: command  action: activate cmd-str:
pcbnew.LengthTuner
11:32:28 AM: Trace: (KICAD_TOOL_STACK) TOOL_MANAGER::dispatchActivation
category: command  action: activate cmd-str: pcbnew.LengthTuner
11:32:28 AM: Trace: (KICAD_TOOL_STACK) TOOL_MANAGER::dispatchActivation
Running tool pcbnew.LengthTuner for event: category: command  action:
activate cmd-str: pcbnew.LengthTuner
11:32:28 AM: Trace: (KICAD_TOOL_STACK) TOOL_MANAGER::processEvent handled:
true  category: command  action: activate cmd-str: pcbnew.LengthTuner
11:32:29 AM: Trace: (KICAD_TOOL_STACK) TOOL_MANAGER::dispatchInternal
handled: true  category: command  action: activate cmd-str:
pcbnew.LengthTuner.TuneDiffPair
11:32:29 AM: Trace: (KICAD_TOOL_STACK) TOOL_MANAGER::dispatchActivation
category: command  action: activate cmd-str: pcbnew.LengthTuner.TuneDiffPair
11:32:29 AM: Trace: (KICAD_TOOL_STACK) TOOL_MANAGER::processEvent handled:
true  category: command  action: activate cmd-str:
pcbnew.LengthTuner.TuneDiffPair


>From the comment about the second search step (action_manager.cpp:101), it
sounds like this was added because of the shifted key problem. This doesn't
seem like a good solution to the problem though, because it causes more
issues with other keys as well (for instance, if nothing is assigned to
shift+A, then it just runs the hotkey for A instead).

Is there a reason not to just skip all keyboard events with the shift
modifier that happen in a wxEVT_CHAR_HOOK event? That way they will
continue processing and generate the wxEVT_CHAR, and we don't have to have
every tool skipping the keyboard events (which I am also seeing some issues
with in the routing tool). I think that would also mean the second hotkey
search step could be removed, fixing the issues it is creating.

I have been doing all of this testing on GTK, so I am not familar with any
of the Windows/Mac processing that it does, so is any of this key
translation also platform specific?

-Ian

On Sun, Jul 28, 2019 at 2:27 PM jp charras <jp.charras@xxxxxxxxxx> wrote:

> Le 28/07/2019 à 02:22, Jeff Young a écrit :
> > I think JP ran into this earlier, so he might know more about it.
>
> In fact issue I h=worked on was the fact the Char events where no
> correctly skipped.
>
> If I correctly understand the issue related by this mail, this is a
> dispatch issue.
>
> I am thinking I already saw this issue.
> The key event dispatch incorrectly try to dispatch the key events:
> For me it try to find an action that matches the corresponding hotkey in
> its lists, regardless the fact the action is attached to the active
> frame (the frame having the focus) or not.
> If this is true, this dispatch behavior is really not correct:
> Only actions depending on the active frame must be taken in account.
> This is what the wxWidgets menu accelerators work.
>
>
> Now, about wxEVT_CHAR_HOOK and wxEVT_CHAR events (and related hotkeys):
> Among differences between these events, there is one major diff:
> wxEVT_CHAR_HOOK identify the keyboard key (i.e. the not shifted key code).
> wxEVT_CHAR identify the key code (depending on the fact the shift key is
> pressed or not).
> They are very different for keys that have 2 very different key codes.
>
> I explain:
>
> On a French keyboard, all "digit" keys have 2 key code (like in many
> other keyboards), but digit keys are shifted.
> For instance the '3' key is also the '"' key.
> In other words '3' and '3' use the same key ( I will call it 3")
> '3' needs to type SHIFT + 3" key
> '"' needs to type the 3" key
>
> Now what about wxEVT_CHAR_HOOK:
> wxEVT_CHAR_HOOK returns the key '"' with the modifier Shift.
> wxEVT_CHAR returns the key '3' with the modifier Shift.
>
> As a result: to show the 3D viewer in PCBNEW:
> - on an English keyboard you use Alt+3
> - on an French keyboard you use Alt+"
>
> And I am not sure this is a bug in wxWidgets:
> some other applications have this annoying behavior, for instance
> Firefox and its zoom commands (but I used also a few other apps showing
> this behavior).
>
> >
> > Another issue might be that at the end of the tool loop the event will
> get skipped which will send it to wxWidgets for processing (at which point
> it will probably come back as a hotkey again).  We might already have logic
> to deal with that, but it’s something to look out for.
> >
> > At the end of the day, though, we need to design around what is
> “right”.  If what’s right breaks other stuff then we need to fix the other
> stuff.
> >
> > Cheers,
> > Jeff.
> >
> >> On 27 Jul 2019, at 17:48, Ian McInerney <Ian.S.McInerney@xxxxxxxx>
> wrote:
> >>
> >> In the tool dispatcher currently, if any action is associated with a
> key combination (even if the action is not handled by any active tools)
> then a key event with that combination will not progress further than the
> dispatchHotKey function in the dispatcher. For instance, this means that
> the letter 'B' will not make it into any of the dispatchInternal calls
> inside any program (e.g. eeschema, pleditor, cvpcb, etc.) because it is
> assigned to a pcbnew action to refill the zones, even when those other
> programs do not use that action at all so it goes unhandled in the
> dispatchHotkey function. That event therefore is inaccessible in any tool
> loops that may be running.
> >>
> >> Is there a reason the dispatchHotkey logic looks only at the fact an
> action with that hotkey exists rather than if any tool has handled the
> associated action? For my work in cvpcb it would be better if it were the
> latter, so that any key events not handled by the tools continue processing
> (e.g. for down/up/left/right keys, single letter keys, etc.). It should be
> possible to know if the action is handled by the hotkey handler, since they
> actions are spawned immediately and the handled return value is then
> available.
> >>
> >> Would a change to this system break any of the existing tool loops?
> e.g., are any unable to cope with receiving key pressed events (I don't
> think any would be problematic, since some keys such as 'C' don't have an
> associated action and would therefore generate key pressed events in them)?
> >>
> >> -Ian
> >> _______________________________________________
> >> Mailing list: https://launchpad.net/~kicad-developers
> >> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> >> Unsubscribe : https://launchpad.net/~kicad-developers
> >> More help   : https://help.launchpad.net/ListHelp
> >
> >
> > _______________________________________________
> > Mailing list: https://launchpad.net/~kicad-developers
> > Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> > Unsubscribe : https://launchpad.net/~kicad-developers
> > More help   : https://help.launchpad.net/ListHelp
> >
>
>
> --
> Jean-Pierre CHARRAS
>
> _______________________________________________
> Mailing list: https://launchpad.net/~kicad-developers
> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
>