kicad-developers team mailing list archive

[jp charras <jp.charras@xxxxxxxxxx>]

Le 31/07/2019 à 15:22, Wayne Stambaugh a écrit :
> 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

I confirm on Windows, if a menu accelerator captures a wxEVT_CHAR_HOOK,
the corresponding wxEVT_CHAR is not generated.

> 
> 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
>>
> 
> _______________________________________________
> 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