kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxx>]

Hey Ian,

If I remember correctly, the windows platform grabs the wxEVT_CHAR_HOOK
events to implement menu accelerators which get priority over the
translated wxEVT_CHAR events and breaks our hotkey implementation when
there is a conflict.  JP can probably shed some more light on this.  I
suspect we could find a better solution where we use the "standard"
wxWidgets menu accelerator paradigm and factor out our hotkey
implementation but it would most likely be a very painful transition.

Cheers,

Wayne

On 7/31/19 5:50 AM, Ian McInerney wrote:
> 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
> <mailto: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
>     <mailto: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
>     <mailto: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
>     <mailto: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
>     <mailto: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
>