Footprint editor Libraries filter doesn't take all characters

[eelik]

Footprint editor's library tree filter acts funnily with some characters. It doesn't seem to accept for example 7 or 0, and H toggles the Highlight mode, i.e. is interpreted as the hotkey.

Application: kicad
Version: (6.0.0-rc1-dev-1198-gbcc8c6425), release build
Libraries:
    wxWidgets 3.0.4
    libcurl/7.61.1 OpenSSL/1.1.1 (WinSSL) zlib/1.2.11 brotli/1.0.6 libidn2/2.0.5 libpsl/0.20.2 (+libidn2/2.0.5) nghttp2/1.34.0
Platform: Windows 7 (build 7601, Service Pack 1), 64-bit edition, 64 bit, Little endian, wxMSW
Build Info:
    wxWidgets: 3.0.4 (wchar_t,wx containers,compatible with 2.8)
    Boost: 1.68.0
    OpenCASCADE Community Edition: 6.9.1
    Curl: 7.61.1
    Compiler: GCC 8.2.0 with C++ ABI 1013

Build settings:
    USE_WX_GRAPHICS_CONTEXT=OFF
    USE_WX_OVERLAY=OFF
    KICAD_SCRIPTING=ON
    KICAD_SCRIPTING_MODULES=ON
    KICAD_SCRIPTING_PYTHON3=OFF
    KICAD_SCRIPTING_WXPYTHON=ON
    KICAD_SCRIPTING_WXPYTHON_PHOENIX=OFF
    KICAD_SCRIPTING_ACTION_MENU=ON
    BUILD_GITHUB_PLUGIN=ON
    KICAD_USE_OCE=ON
    KICAD_USE_OCC=OFF
    KICAD_SPICE=ON

[Wayne Stambaugh (stambaughw)]

I'm not having this issue with 0 or 7 but I am seeing the highlight tool state toggle with the 'h' (but not 'H') key so it looks like the hot key filters are grabbing the keystrokes from the filter control. Have you remapped some of your hotkeys to 0 and 7? This would explain why I'm not seeing that.

[eelik (eelik)]

No, I've done nothing with hotkeys config.

[Seth Hillbrand (sethh)]

This appears to be MSW only. @eelik do the 0-7 work on either the numpad or the number row?

[eelik (eelik)]

Numpad seems to work, the number row not. But H still triggers the High contrast mode.

[Jacob E. F. Overgaard (jacobefo)]

I can confirm I have this issue on:

Application: kicad
Version: (6.0.0-rc1-dev-1521-g81a0ab4d7), release build
Libraries:
    wxWidgets 3.0.4
    libcurl/7.61.1 OpenSSL/1.1.1 (WinSSL) zlib/1.2.11 brotli/1.0.6 libidn2/2.0.5 libpsl/0.20.2 (+libidn2/2.0.5) nghttp2/1.34.0
Platform: Windows 8 (build 9200), 64-bit edition, 64 bit, Little endian, wxMSW
Build Info:
    wxWidgets: 3.0.4 (wchar_t,wx containers,compatible with 2.8)
    Boost: 1.68.0
    OpenCASCADE Community Edition: 6.9.1
    Curl: 7.61.1
    Compiler: GCC 8.2.0 with C++ ABI 1013

Build settings:
    USE_WX_GRAPHICS_CONTEXT=OFF
    USE_WX_OVERLAY=OFF
    KICAD_SCRIPTING=ON
    KICAD_SCRIPTING_MODULES=ON
    KICAD_SCRIPTING_PYTHON3=OFF
    KICAD_SCRIPTING_WXPYTHON=ON
    KICAD_SCRIPTING_WXPYTHON_PHOENIX=OFF
    KICAD_SCRIPTING_ACTION_MENU=ON
    BUILD_GITHUB_PLUGIN=ON
    KICAD_USE_OCE=ON
    KICAD_USE_OCC=OFF
    KICAD_SPICE=ON

Using 'h' toggles high contrast display mode, while 'H' enters the letter 'H' into the filter field.
I have not changed any hotkeys intentionally.

[Seth Hillbrand (sethh)]

@Wayne- I suspect that this bug is the result of 53b1ec81. Can you try reverting that one (or just un-hooking wxEVT_CHAR_HOOK from OnKeyEvent()) and see if it fixes this?

[Wayne Stambaugh (stambaughw)]

@Seth, that patch was reverted in a1acec5f so it cannot be the issue. The tool event dispatcher never sees the event when the filter edit control has the focus. Using the character tracing with the filter edit control having the focus I get the following debug statement:

[8960] (KICAD_KEY_EVENTS) EDA_DRAW_FRAME::OnCharHook Hook 'H' 72 ---- 72 (U+0048) 72 0x00230001 ( 59, 5)

When the canvas has the focus I get:

[8960] (KICAD_KEY_EVENTS) TOOL_DISPATCHER::DispatchWxEvent Hook ALT 307 -A-- 0 (U+0000) 18 0x20380001 ( 145, 344)
[8960] (KICAD_KEY_EVENTS) EDA_DRAW_FRAME::OnCharHook Hook 'H' 72 ---- 72 (U+0048) 72 0x00230001 ( 145, 344)

Once I remove the char event hooks from EDA_DRAW_FRAME and the filter edit control has the focus I get:

[11912] (KICAD_KEY_EVENTS) TOOL_DISPATCHER::DispatchWxEvent Hook 'H' 72 ---- 72 (U+0048) 72 0x00230001 ( 72, 230)

It looks like the tool dispatcher is the issue and is stealing the character from the event loop but maybe I'm missing something. I'm not sure handling the wxCharHookEvent directly is ever a good idea. My guess is fixing this is not going to be trivial. Once we remove all of the legacy key handling code from all of KiCad, we might have a fighting chance to get it right. I'm sure if I start removing wxCharHookEvents from the tool dispatcher, all hell will break loose. We may have to defer this one.

[Seth Hillbrand (sethh)]

Hi Wayne-

Sorry, I should have been specific. I meant line 67 of legacy_wx/eda_draw_panel.cpp. That remains from 53b1ec81 and will not pass the event if the GeneralControl call handles the key.

[Wayne Stambaugh (stambaughw)]

On 2/17/19 6:00 PM, Seth Hillbrand wrote:
> Hi Wayne-
>
> Sorry, I should have been specific. I meant line 67 of
> legacy_wx/eda_draw_panel.cpp. That remains from 53b1ec81 and will not
> pass the event if the GeneralControl call handles the key.
>

I thought GeneralControl was only in play in the legacy canvas. I'm
seeing this with the GAL canvas enabled. I did comment out the event
handler in legacy_wx/eda_draw_panel.cpp and it didn't make any difference.

[Seth Hillbrand (sethh)]

Thanks. If you have a chance, can you test the attached patch on Windows?

[Wayne Stambaugh (stambaughw)]

@Seth, no dice. I tried this earlier today along with commenting out all of the char hook events and that didn't resolve the issue. One thing we have to do after we remove the legacy canvas is to seriously reconsider using EVT_CHAR_HOOK. Your solution looks correct in theory because EVT_CHAR_HOOK picks apart the key up and down events and prevent the EVT_CHAR from forming properly even when Skip() is called. I wonder if binding the edit control char events to the tree control is the issue. It might be a better solution to pass some type of event to the parent window to indicate that the tree view needs to be updated.

[Seth Hillbrand (sethh)]

Well, bother. I guess we'll need to think about this more deeply. As in, why is it only some hotkeys that are trapped by this? Is the isSpecialKey() routine picking up some keys in addition to the standard?

If you change the hotkey for High Contrast to a different key, does the problem follow the hotkey or the character?

[eelik (eelik)]

If I change the hotkey then the problem follows the new hotkey. For example changing it to J instead of H makes H acting normally but J toggling the high contrast mode.

[Seth Hillbrand (sethh)]

Thanks eelik. I admit, I was not expecting that.

Do other legacy hotkeys like '{' and '}' also get suppressed?

[jean-pierre charras (jp-charras)]

@Seth,
the root issue (on Windows) is the fact any accelerator key (or a hotkey added to a menuitem as accelerator) in the main menubar has the priority to any other window.

It is captured by the wxEVT_CHAR_HOOK event handler of parent windows.
Therefore if a menuitem (in the main menu, always activated) captures the key code of the wxEVT_CHAR_HOOK, the corresponding wxEVT_CHAR is never fired on Windows.

This issue is usually noticed when "printable" keys are used as accelerators.
(Not printable keys like Ctrl+xx or alt+xx are captured, but you never use them in wxTextCtrl so do not see this issue)

And if you capture this event (do not skip it), the wxEVT_CHAR is not fired, and the wxTextCtrl never receive a wxEVT_CHAR event.

Note also, for the same key of the keyboard, the key code returned by wxEVT_CHAR_HOOK event and wxEVT_CHAR event are not the same code.
This issue is not specific to the wxTextCtrl filter: this is a problem for all our hotkey management, and it does not depend on the canvas type.

[eelik (eelik)]

{ and } aren't written to the filter field, [ and ] are. (Both are typed with AltGr+number in the Finnish layout.)

Other numbers work but 7 and 0 don't work. They aren't tied to anything in Preferences -> Hotkeys. But { is AltGr+7 and } is AltGr+0.

From the letters only 'h' doesn't seem to work. All letters work with Shift, including H.

Behind the number row with the Shift key everything works: !"#¤%&/()=
(slash '/' is Shift+7, = is Shift+0)

Behind the number row with AltGr everything but {} works: @£$€[]

Other special characters with Shift or AltGr seem to work.

In the numpad all numbers work, including 7 and 0, as I already told.

[eelik (eelik)]

jp answered while I was writing. Indeed, h, { and } are the only printable characters which are found in the Footprint Editor's menu, View->Contrast Mode.

[eelik (eelik)]

Except that I can't find 0 or 7 there.

[Wayne Stambaugh (stambaughw)]

JP, is correct. I forgot about the menu accelerator behavior on windows. We should not use or allow alphanumeric keys as menu accelerators without a modifier key. Developers still seem to be struggling with differentiating between menu accelerators and hot keys. Toggling the high contrast mode is not a hot key action. It is a menu accelerator action. I don't see any other way to resolve this without digging deep into the bowels of wxWidgets windows event handling and change it. I would rather not even think about attempting that. I changed the high contrast key to Ctrl+H and it resolved the issue. I'm sure there are other alphanumeric keys lurking as menu accelerator keys.

[Seth Hillbrand (sethh)]

@JP- Makes sense. But Wayne tried unhooking all the CHAR_HOOK handlers (#11) without success.

[Wayne Stambaugh (stambaughw)]

@Seth, most likely wxWidgets is filtering the menu accelerator events in wxApp::FilterEvent() which gets called before any other event handlers so EVT_CHAR_HOOK doesn't make any difference in this regard. The easiest way to fix this is to just change any hot key definition that uses a single valid footprint name character and is not a true hot key to use some modifier key. Of course this doesn't prevent users from changing them to single characters so that will need to be addressed at least on windows.

[Wayne Stambaugh (stambaughw)]

@eelik, have you changed any of your hot key assignments? If not, I cannot explain why you are also having an issue with '0' and '7'. I'm not seeing this with the default hot key mapping.

[Seth Hillbrand (sethh)]

I'd guess that it's because he's using a Finnish key layout that has '{' and '}' overlaid onto 7 and 0 and wx hotkey handler is masking AltGr.

[jean-pierre charras (jp-charras)]

In fact there are a (ugly) trick on Windows:
When a hotkey (handled by our EVT_CHAR event handler), is added in a menuitem, it is seen as accelerator key, but mainly we just want it shown as a comment.
For instance the H hotkey is added as "\tH" string to the menu, and therefore is seen by wxWidgets as a menu accelerator.
The trick is to add: "\t H" or "\tH " (note the space char) and the hotkey is no longer seen as a accelerator key, and is not captured by the menuitem.
Of course it imply accelerator keys do not exist in menus, all keys are hotkeys managed by our EVT_CHAR event handler

[Wayne Stambaugh (stambaughw)]

@JP, I would prefer we not use that trick although it is handy to know. All this will do is continue to confuse users and developers of the differences between hot keys and menu accelerators. I would prefer that we change the toggle high contrast mode hot key to ctrl+h. We still need to address the issue of users remapping keys. On windows, I'm OK with preventing mapping filter keys to menu accelerators will cause this issue. I already have the changed hot key definition in my repo so I can fix this if there are no objections. Existing users are going to have to reset their footprint editor key maps to resolve the issue. I didn't see any other hot key definitions that violate this condition for the footprint editor, symbol editor, or cvpcb. No other main frame windows have text edit controls as children so it wont be an issue for the schematic and board editors.

[Wayne Stambaugh (stambaughw)]

I just push a fix for this at least for new users. Existing users will have to reset their footprint editor key map to fix the issue.

@eelik, please try changing your missing filter control keys to use a key modifier to prevent the key presses from being hijacked by wxWidgets on windows. If this doesn't resolve your issue, please set the status of this bug report back to triaged.

[eelik (eelik)]

If I change { and } to [ and ] the missing numbers change from 7 and 0 to 8 and 9, respectively, so it's about the keyboard layout just like Seth guessed.

[Wayne Stambaugh (stambaughw)]

@eelik, can you not use a modifier key like Ctrl or Alt or Ctrl+Alt? This is going to be a must in your case. Not being able to use 0-9 for filtering is going to be problematic.

[eelik (eelik)]

No problems, I haven't used those hotkeys anyways.