kicad-developers team mailing list archive

[Chris Pavlina <pavlina.chris@xxxxxxxxx>]

There is an old bug that people turned up while testing my new hotkey 
editor, attached is a patch that fixes it. This patch pokes into the 
main hotkey handling code, so I really want as many people to test it as 
possible - it's a relatively minor bug, I don't want to go introducing 
twelve regressions to fix one small bug. Here's what I want to keep an 
eye out for:

- Hotkeys (Ctrl+Tab), (Tab), and (Ctrl+I) are all independent and 
  distinguished from each other, both in the hotkey editor and in actual 
  use. Make sure all of them work, and make sure none of them answers 
  for the others.

- Hotkeys that are *not* handled by the main hotkey code (for example, 
  menu keys like Alt+F to call up the File menu) are not affected.

- Behavior on OSX, with its weird Ctrl mapping, is not broken (or, at 
  least, is no more broken than usual ;)

I have tested on Linux and Windows 10, though more thorough testing on 
those platforms is welcome.

Patch/bug summary:

[PATCH] Fix (Ctrl)+(ASCII control key) hotkey handling

wxWidgets has quirks with how it handles these keys. For example, in
wxEVT_CHAR, Ctrl+Tab and Ctrl+I are indistinguishable.

- Modify the special wxEVT_CHAR_HOOK handler from WIDGET_HOTKEY_LIST to
  handle this case as well as the other funny cases it already handles.

- Factor this handler out into a function in hotkeys_basic.h for use
  elsewhere.

- Add this handler to the central hotkey handler, remove existing
  (buggy) ASCII control key handling.

Thanks for testing.

-- 
Chris

>From 86ed9ecfe6cdada8cae8ce6089e9fe2e600c2f18 Mon Sep 17 00:00:00 2001
From: Chris Pavlina <pavlina.chris@xxxxxxxxx>
Date: Wed, 20 Jan 2016 21:10:46 -0500
Subject: [PATCH] Fix (Ctrl)+(ASCII control key) hotkey handling

wxWidgets has quirks with how it handles these keys. For example, in
wxEVT_CHAR, Ctrl+Tab and Ctrl+I are indistinguishable.

- Modify the special wxEVT_CHAR_HOOK handler from WIDGET_HOTKEY_LIST to
  handle this case as well as the other funny cases it already handles.

- Factor this handler out into a function in hotkeys_basic.h for use
  elsewhere.

- Add this handler to the central hotkey handler, remove existing
  (buggy) ASCII control key handling.
---
 common/draw_panel.cpp                 | 21 ++++++-------
 common/hotkeys_basic.cpp              | 56 +++++++++++++++++++++++++++++++++++
 common/widgets/widget_hotkey_list.cpp | 49 +++---------------------------
 include/hotkeys_basic.h               | 34 ++++++++++++++++++++-
 include/widgets/widget_hotkey_list.h  |  2 +-
 5 files changed, 105 insertions(+), 57 deletions(-)

diff --git a/common/draw_panel.cpp b/common/draw_panel.cpp
index 063c5db..6f8733f 100644
--- a/common/draw_panel.cpp
+++ b/common/draw_panel.cpp
@@ -39,6 +39,7 @@
 #include <class_base_screen.h>
 #include <draw_frame.h>
 #include <view/view_controls.h>
+#include <hotkeys_basic.h>
 
 #include <kicad_device_context.h>
 
@@ -1384,12 +1385,13 @@ void EDA_DRAW_PANEL::OnMouseEvent( wxMouseEvent& event )
 
 void EDA_DRAW_PANEL::OnCharHook( wxKeyEvent& event )
 {
-    event.Skip();
+    if( HotkeyCharHookHandler( event ) )
+        OnKeyEvent( event );
 }
 
 void EDA_DRAW_PANEL::OnKeyEvent( wxKeyEvent& event )
 {
-    int localkey;
+    EDA_KEY localkey;
     wxPoint pos;
 
     localkey = event.GetKeyCode();
@@ -1409,13 +1411,6 @@ void EDA_DRAW_PANEL::OnKeyEvent( wxKeyEvent& event )
         break;
     }
 
-    /* Normalize keys code to easily handle keys from Ctrl+A to Ctrl+Z
-     * They have an ascii code from 1 to 27 remapped
-     * to GR_KB_CTRL + 'A' to GR_KB_CTRL + 'Z'
-     */
-    if( event.ControlDown() && localkey >= WXK_CONTROL_A && localkey <= WXK_CONTROL_Z )
-        localkey += 'A' - 1;
-
     /* Disallow shift for keys that have two keycodes on them (e.g. number and
      * punctuation keys) leaving only the "letter keys" of A-Z.
      * Then, you can have, e.g. Ctrl-5 and Ctrl-% (GB layout)
@@ -1445,7 +1440,13 @@ void EDA_DRAW_PANEL::OnKeyEvent( wxKeyEvent& event )
     GetParent()->SetMousePosition( pos );
 
     if( !GetParent()->GeneralControl( &DC, pos, localkey ) )
-        event.Skip();
+    {
+        // If this was a wxEVT_CHAR_HOOK that passed on to us, we do not want
+        // to 'skip', as this will result in the matching wxEVT_CHAR being
+        // generated (only on some platforms!) and double-fire the event.
+        if( event.GetEventType() != wxEVT_CHAR_HOOK )
+            event.Skip();
+    }
 }
 
 
diff --git a/common/hotkeys_basic.cpp b/common/hotkeys_basic.cpp
index 04dfd7f..6774e77 100644
--- a/common/hotkeys_basic.cpp
+++ b/common/hotkeys_basic.cpp
@@ -844,3 +844,59 @@ void AddHotkeyConfigMenu( wxMenu* aMenu )
                  _( "Hotkeys configuration and preferences" ),
                  KiBitmap( hotkeys_xpm ) );
 }
+
+
+bool HotkeyCharHookHandler( wxKeyEvent& aEvent )
+{
+    // On certain platforms, EVT_CHAR_HOOK is the only handler that receives
+    // certain "special" keys. However, it doesn't always receive "normal"
+    // keys correctly. For example, with a US keyboard, it sees ? as shift+/.
+    //
+    // Untangling these incorrect keys would be too much trouble, so we bind
+    // both events, and simply skip the EVT_CHAR_HOOK if it receives a
+    // "normal" key.
+
+    const enum wxKeyCode skipped_keys[] =
+    {
+        WXK_NONE,    WXK_SHIFT,  WXK_ALT, WXK_CONTROL, WXK_CAPITAL,
+        WXK_NUMLOCK, WXK_SCROLL, WXK_RAW_CONTROL
+    };
+
+    int key = aEvent.GetKeyCode();
+
+    for( size_t i = 0; i < DIM( skipped_keys ); ++i )
+    {
+        if( key == skipped_keys[i] )
+            return false;
+    }
+
+    // Some control keys map to Ctrl+letter (e.g. Tab is Ctrl+I) in OnChar,
+    // but not OnCharHook. This leads to incorrect recognition of
+    // Ctrl+controlkey (e.g. Ctrl+Tab is seen as just Ctrl+I). Detect
+    // this and handle here instead.
+
+    bool ctrl_alphakey = ( ( key >= 'A' && key <= 'Z' ) || ( key >= 'a' && key <= 'z' ) )
+            && aEvent.ControlDown();
+
+    bool ctrl_ctrlkey = ( key >= WXK_CONTROL_A && key <= WXK_CONTROL_Z )
+            && aEvent.ControlDown();
+
+    if( key <= 255 && isprint( key ) && !isspace( key ) && !ctrl_ctrlkey && !ctrl_alphakey )
+    {
+        // Let EVT_CHAR handle this one
+        aEvent.DoAllowNextEvent();
+
+        // On Windows, wxEvent::Skip must NOT be called.
+        // On Linux and OSX, wxEvent::Skip MUST be called.
+        // No, I don't know why.
+#ifndef __WXMSW__
+        aEvent.Skip();
+#endif
+
+        return false;
+    }
+    else
+    {
+        return true;
+    }
+}
diff --git a/common/widgets/widget_hotkey_list.cpp b/common/widgets/widget_hotkey_list.cpp
index 3813670..cbaca2e 100644
--- a/common/widgets/widget_hotkey_list.cpp
+++ b/common/widgets/widget_hotkey_list.cpp
@@ -152,44 +152,8 @@ public:
 
     void OnCharHook( wxKeyEvent& aEvent )
     {
-        // On certain platforms, EVT_CHAR_HOOK is the only handler that receives
-        // certain "special" keys. However, it doesn't always receive "normal"
-        // keys correctly. For example, with a US keyboard, it sees ? as shift+/.
-        //
-        // Untangling these incorrect keys would be too much trouble, so we bind
-        // both events, and simply skip the EVT_CHAR_HOOK if it receives a
-        // "normal" key.
-
-        const enum wxKeyCode skipped_keys[] =
-        {
-            WXK_NONE,    WXK_SHIFT,  WXK_ALT, WXK_CONTROL, WXK_CAPITAL,
-            WXK_NUMLOCK, WXK_SCROLL, WXK_RAW_CONTROL
-        };
-
-        int key = aEvent.GetKeyCode();
-
-        for( size_t i = 0; i < sizeof( skipped_keys ) / sizeof( skipped_keys[0] ); ++i )
-        {
-            if( key == skipped_keys[i] )
-                return;
-        }
-
-        if( key <= 255 && isprint( key ) && !isspace( key ) )
-        {
-            // Let EVT_CHAR handle this one
-            aEvent.DoAllowNextEvent();
-
-            // On Windows, wxEvent::Skip must NOT be called.
-            // On Linux and OSX, wxEvent::Skip MUST be called.
-            // No, I don't know why.
-#ifndef __WXMSW__
-            aEvent.Skip();
-#endif
-        }
-        else
-        {
+        if( HotkeyCharHookHandler( aEvent ) )
             OnChar( aEvent );
-        }
     }
 
 
@@ -302,7 +266,7 @@ void WIDGET_HOTKEY_LIST::EditItem( wxTreeListItem aItem )
     wxString    current_key = GetItemText( aItem, 1 );
 
     wxKeyEvent key_event = HK_PROMPT_DIALOG::PromptForKey( GetParent(), name, current_key );
-    long key = MapKeypressToKeycode( key_event );
+    EDA_KEY key = MapKeypressToKeycode( key_event );
 
     if( hkdata && key )
     {
@@ -634,9 +598,9 @@ bool WIDGET_HOTKEY_LIST::TransferDataFromControl()
 }
 
 
-long WIDGET_HOTKEY_LIST::MapKeypressToKeycode( const wxKeyEvent& aEvent )
+EDA_KEY WIDGET_HOTKEY_LIST::MapKeypressToKeycode( const wxKeyEvent& aEvent )
 {
-    long key = aEvent.GetKeyCode();
+    EDA_KEY key = ( EDA_KEY ) aEvent.GetKeyCode();
 
     if( key == WXK_ESCAPE )
     {
@@ -647,11 +611,6 @@ long WIDGET_HOTKEY_LIST::MapKeypressToKeycode( const wxKeyEvent& aEvent )
         if( key >= 'a' && key <= 'z' )    // convert to uppercase
             key = key + ('A' - 'a');
 
-        // Remap Ctrl A (=1+GR_KB_CTRL) to Ctrl Z(=26+GR_KB_CTRL)
-        // to GR_KB_CTRL+'A' .. GR_KB_CTRL+'Z'
-        if( aEvent.ControlDown() && key >= WXK_CONTROL_A && key <= WXK_CONTROL_Z )
-            key += 'A' - 1;
-
         /* Disallow shift for keys that have two keycodes on them (e.g. number and
          * punctuation keys) leaving only the "letter keys" of A-Z.
          * Then, you can have, e.g. Ctrl-5 and Ctrl-% (GB layout)
diff --git a/include/hotkeys_basic.h b/include/hotkeys_basic.h
index 089fcce..0794fe1 100644
--- a/include/hotkeys_basic.h
+++ b/include/hotkeys_basic.h
@@ -57,7 +57,7 @@ extern wxString g_CommonSectionTag;
 class EDA_HOTKEY
 {
 public:
-    int      m_KeyCode;      // Key code (ascii value for ascii keys or wxWidgets code for function key
+    EDA_KEY  m_KeyCode;      // Key code (ascii value for ascii keys or wxWidgets code for function key
     wxString m_InfoMsg;      // info message.
     int      m_Idcommand;    // internal id for the corresponding command (see hotkey_id_commnand list)
     int      m_IdMenuEvent;  // id to call the corresponding event (if any) (see id.h)
@@ -244,4 +244,36 @@ enum common_hotkey_id_commnand {
     HK_COMMON_END
 };
 
+/**
+ * Function HotkeyCharHookHandler
+ * Trap certain "special" keys in wxEVT_CHAR_HOOK, modifying them as needed
+ * for use as hotkeys. Some keys will automatically be forwarded to
+ * wxEVT_CHAR, others must be passed there by the class using this function.
+ * An example implementation is:
+ *
+ *      void OnCharHook( wxKeyEvent& aEvent )
+ *      {
+ *          if( HotkeyCharHookHandler( aEvent ) )
+ *          {
+ *              OnChar( aEvent );
+ *          }
+ *      }
+ *
+ *      void OnChar( wxKeyEvent& aEvent )
+ *      {
+ *          do_normal_hotkey_processing();
+ *
+ *          // wxEvent::Skip() on wxEVT_CHAR_HOOK could trigger a double-fire
+ *          // of events on some platforms. Don't do that.
+ *
+ *          if( we_want_to_skip && aEvent.GetEventType() != wxEVT_CHAR_HOOK )
+ *              aEvent.Skip();
+ *      }
+ *
+ *
+ * @param aEvent - the key event received by OnCharHook
+ * @return true iff the event should be forwarded to OnChar by the caller.
+ */
+bool HotkeyCharHookHandler( wxKeyEvent& aEvent );
+
 #endif // HOTKEYS_BASIC_H
diff --git a/include/widgets/widget_hotkey_list.h b/include/widgets/widget_hotkey_list.h
index bfaf010..443c10e 100644
--- a/include/widgets/widget_hotkey_list.h
+++ b/include/widgets/widget_hotkey_list.h
@@ -196,7 +196,7 @@ public:
      * Static method MapKeypressToKeycode
      * Map a keypress event to the correct key code for use as a hotkey.
      */
-    static long MapKeypressToKeycode( const wxKeyEvent& aEvent );
+    static EDA_KEY MapKeypressToKeycode( const wxKeyEvent& aEvent );
 };
 
 #endif // __widget_hotkey_list__
-- 
2.7.0