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 <[email protected]> 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 <[email protected] > <mailto:[email protected]>> 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 <[email protected] > >> <mailto:[email protected]> > >> <mailto:[email protected] <mailto:[email protected]>>> 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 <[email protected] > >> <mailto:[email protected]> > >> <mailto:[email protected] <mailto:[email protected]>>> > >> 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 : [email protected] > >> <mailto:[email protected]> > >> <mailto:[email protected] > >> <mailto:[email protected]>> > >> >> 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 : [email protected] > >> <mailto:[email protected]> > >> <mailto:[email protected] > >> <mailto:[email protected]>> > >> > 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 : [email protected] > >> <mailto:[email protected]> > >> <mailto:[email protected] > >> <mailto:[email protected]>> > >> 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 : [email protected] > >> <mailto:[email protected]> > >> 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 : [email protected] > > <mailto:[email protected]> > > 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 : [email protected] > <mailto:[email protected]> > 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 : [email protected] > Unsubscribe : https://launchpad.net/~kicad-developers > More help : https://help.launchpad.net/ListHelp
_______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
