kicad-developers team mailing list archive

[Jeff Young <jeff@xxxxxxxxx>]

Hi Ian,

Some work I did to make sure wxEVT_CHAR_HOOK events get skipped should help allow the removal of the older hack.  I think we should at least try it out.

Note that we’ve now added ctrl+W to ctrl+Q, but those are probably the only two that need special-case processing.

Cheers,
Jeff.


> On 2 Aug 2019, at 14:01, Ian McInerney <Ian.S.McInerney@xxxxxxxx> wrote:
> 
> Ok, from the sound of it the accelerator keys will be the blocking issue for solving the shifted key problem. The discussion from this report (https://bugs.launchpad.net/kicad/+bug/1809929 <https://bugs.launchpad.net/kicad/+bug/1809929>) should probably be revisited then since a lot of work has been done to the hotkey system since then.
> 
> Looking at how things currently are implemented, the only accelerator table entries in the code base are for the ctrl+Q quiting of the program. The keys shown in the menu items are simply the hotkeys assigned to the actions and there are no actual accelerators associated with them, so there should be no hijacking of the key events between the char hook and char events currently.
> 
> PS: GTK also has the accelerator key acting before the char event, so I think that is for every platform.
> 
> -Ian
> 
> 
> 
> On Wed, Jul 31, 2019 at 4:27 PM jp charras <jp.charras@xxxxxxxxxx <mailto:jp.charras@xxxxxxxxxx>> wrote:
> 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 <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>
> >> <mailto: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>
> >>     <mailto: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 <https://launchpad.net/~kicad-developers>
> >>     >> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
> >>     <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>>
> >>     >> Unsubscribe : https://launchpad.net/~kicad-developers <https://launchpad.net/~kicad-developers>
> >>     >> More help   : https://help.launchpad.net/ListHelp <https://help.launchpad.net/ListHelp>
> >>     >
> >>     >
> >>     > _______________________________________________
> >>     > Mailing list: https://launchpad.net/~kicad-developers <https://launchpad.net/~kicad-developers>
> >>     > Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
> >>     <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>>
> >>     > Unsubscribe : https://launchpad.net/~kicad-developers <https://launchpad.net/~kicad-developers>
> >>     > More help   : https://help.launchpad.net/ListHelp <https://help.launchpad.net/ListHelp>
> >>     >
> >>
> >>
> >>     -- 
> >>     Jean-Pierre CHARRAS
> >>
> >>     _______________________________________________
> >>     Mailing list: https://launchpad.net/~kicad-developers <https://launchpad.net/~kicad-developers>
> >>     Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
> >>     <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>>
> >>     Unsubscribe : https://launchpad.net/~kicad-developers <https://launchpad.net/~kicad-developers>
> >>     More help   : https://help.launchpad.net/ListHelp <https://help.launchpad.net/ListHelp>
> >>
> >>
> >> _______________________________________________
> >> Mailing list: https://launchpad.net/~kicad-developers <https://launchpad.net/~kicad-developers>
> >> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
> >> Unsubscribe : https://launchpad.net/~kicad-developers <https://launchpad.net/~kicad-developers>
> >> More help   : https://help.launchpad.net/ListHelp <https://help.launchpad.net/ListHelp>
> >>
> > 
> > _______________________________________________
> > Mailing list: https://launchpad.net/~kicad-developers <https://launchpad.net/~kicad-developers>
> > Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
> > Unsubscribe : https://launchpad.net/~kicad-developers <https://launchpad.net/~kicad-developers>
> > More help   : https://help.launchpad.net/ListHelp <https://help.launchpad.net/ListHelp>
> > 
> 
> 
> -- 
> Jean-Pierre CHARRAS
> 
> _______________________________________________
> Mailing list: https://launchpad.net/~kicad-developers <https://launchpad.net/~kicad-developers>
> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
> Unsubscribe : https://launchpad.net/~kicad-developers <https://launchpad.net/~kicad-developers>
> More help   : https://help.launchpad.net/ListHelp <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