From 2a08afb3694dd23858533d2b21b6fd9adf2eddbc Mon Sep 17 00:00:00 2001 From: Ian McInerney Date: Sun, 25 Aug 2019 15:22:15 +0200 Subject: [PATCH 1/3] Replace fail msg with logdebug in the GTK accelerator creation --- src/gtk/menu.cpp | 48 ++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 38 insertions(+), 10 deletions(-) diff --git a/src/gtk/menu.cpp b/src/gtk/menu.cpp index 67c2d902ca..3065fa2564 100644 --- a/src/gtk/menu.cpp +++ b/src/gtk/menu.cpp @@ -1062,25 +1062,37 @@ static wxString GetGtkHotKey( const wxMenuItem& item ) if ( flags ) hotkey << wxT("Up" ); else - wxFAIL_MSG( wxT("The Up key must have modifiers to be an accelerator key") ); + { + wxLogDebug( wxT("The Up key must have modifiers to be an accelerator key") ); + hotkey.Clear(); + } break; case WXK_DOWN: if ( flags ) hotkey << wxT("Down" ); else - wxFAIL_MSG( wxT("The Down key must have modifiers to be an accelerator key") ); + { + wxLogDebug( wxT("The Down key must have modifiers to be an accelerator key") ); + hotkey.Clear(); + } break; case WXK_LEFT: if ( flags ) hotkey << wxT("Left" ); else - wxFAIL_MSG( wxT("The Left key must have modifiers to be an accelerator key") ); + { + wxLogDebug( wxT("The Left key must have modifiers to be an accelerator key") ); + hotkey.Clear(); + } break; case WXK_RIGHT: if ( flags ) hotkey << wxT("Right" ); else - wxFAIL_MSG( wxT("The Right key must have modifiers to be an accelerator key") ); + { + wxLogDebug( wxT("The Right key must have modifiers to be an accelerator key") ); + hotkey.Clear(); + } break; case WXK_PAGEUP: hotkey << wxT("Page_Up" ); @@ -1156,25 +1168,37 @@ static wxString GetGtkHotKey( const wxMenuItem& item ) if ( flags ) hotkey << wxT("KP_Up" ); else - wxFAIL_MSG( wxT("The KP_Up key must have modifiers to be an accelerator key") ); + { + wxLogDebug( wxT("The KP_Up key must have modifiers to be an accelerator key") ); + hotkey.Clear(); + } break; case WXK_NUMPAD_DOWN: if ( flags ) hotkey << wxT("KP_Down" ); else - wxFAIL_MSG( wxT("The KP_Down key must have modifiers to be an accelerator key") ); + { + wxLogDebug( wxT("The KP_Down key must have modifiers to be an accelerator key") ); + hotkey.Clear(); + } break; case WXK_NUMPAD_LEFT: if ( flags ) hotkey << wxT("KP_Left" ); else - wxFAIL_MSG( wxT("The KP_Left key must have modifiers to be an accelerator key") ); + { + wxLogDebug( wxT("The KP_Left key must have modifiers to be an accelerator key") ); + hotkey.Clear(); + } break; case WXK_NUMPAD_RIGHT: if ( flags ) hotkey << wxT("KP_Right" ); else - wxFAIL_MSG( wxT("The KP_Right key must have modifiers to be an accelerator key") ); + { + wxLogDebug( wxT("The KP_Right key must have modifiers to be an accelerator key") ); + hotkey.Clear(); + } break; case WXK_NUMPAD_PAGEUP: hotkey << wxT("KP_Page_Up" ); @@ -1258,7 +1282,9 @@ static wxString GetGtkHotKey( const wxMenuItem& item ) case WXK_SPECIAL13: case WXK_SPECIAL14: case WXK_SPECIAL15: case WXK_SPECIAL16: case WXK_SPECIAL17: case WXK_SPECIAL18: case WXK_SPECIAL19: case WXK_SPECIAL20: - wxFAIL_MSG( wxT("Unsupported keyboard accelerator key") ); + wxLogDebug( wxString::Format( wxT("Unsupported keyboard accelerator key: %s"), + accel->ToString() ) ); + hotkey.Clear(); break; // if there are any other keys wxAcceleratorEntry::Create() may @@ -1276,7 +1302,9 @@ static wxString GetGtkHotKey( const wxMenuItem& item ) } } - wxFAIL_MSG( wxT("unknown keyboard accel") ); + wxLogDebug( wxString::Format( wxT("unknown keyboard accelerator key code: %i"), + code ) ); + hotkey.Clear(); } delete accel; From e961307043f0ff1a877a68c6dbd155f8d7e565bf Mon Sep 17 00:00:00 2001 From: Ian McInerney Date: Sun, 25 Aug 2019 15:24:44 +0200 Subject: [PATCH 2/3] Add compatibility warning for invalid GTK accelerator keys --- include/wx/accel.h | 8 +++- src/common/accelcmn.cpp | 90 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+), 1 deletion(-) diff --git a/include/wx/accel.h b/include/wx/accel.h index c1f78d58ed..c0f23ca534 100644 --- a/include/wx/accel.h +++ b/include/wx/accel.h @@ -53,7 +53,9 @@ public: , m_keyCode(keyCode) , m_command(cmd) , m_item(item) - { } + { + ValidateKey(flags, keyCode); + } // create accelerator corresponding to the specified string, return NULL if // string couldn't be parsed or a pointer to be deleted by the caller @@ -61,6 +63,8 @@ public: void Set(int flags, int keyCode, int cmd, wxMenuItem *item = NULL) { + ValidateKey(flags, keyCode); + m_flags = flags; m_keyCode = keyCode; m_command = cmd; @@ -118,6 +122,8 @@ public: private: wxString AsPossiblyLocalizedString(bool localized) const; + static bool ValidateKey(int flags, int keycode); + // common part of Create() and FromString() static bool ParseAccel(const wxString& str, int *flags, int *keycode); diff --git a/src/common/accelcmn.cpp b/src/common/accelcmn.cpp index 820985df26..bc8937a2b8 100644 --- a/src/common/accelcmn.cpp +++ b/src/common/accelcmn.cpp @@ -156,6 +156,96 @@ static int IsNumberedAccelKey(const wxString& str, return prefixCode + num - first; } +// static +bool wxAcceleratorEntry::ValidateKey(int flags, int keycode) +{ + wxString keyname; + if ( keycode ) + { + for ( size_t n = 0; n < WXSIZEOF(wxKeyNames); n++ ) + { + const wxKeyName& kn = wxKeyNames[n]; + if ( kn.code == keycode ) + { + keyname = kn.name; + break; + } + } + + if ( keyname.IsEmpty() && keycode >= WXK_SPECIAL1 && keycode <= WXK_SPECIAL20 ) + keyname << wxS("SPECIAL") << keycode - WXK_SPECIAL1 + 1; + } + + bool valid = true; + switch ( keycode ) + { + /* + The following keycodes must have modifier keys to be valid on GTK + */ + case WXK_UP: + case WXK_DOWN: + case WXK_LEFT: + case WXK_RIGHT: + case WXK_NUMPAD_UP: + case WXK_NUMPAD_DOWN: + case WXK_NUMPAD_LEFT: + case WXK_NUMPAD_RIGHT: + if ( !flags ) + { + valid = false; + wxLogDebug( wxString::Format( wxT("Compatibility issue: %s key must have modifiers to be an accelerator key on GTK"), + keyname ) ); + } + break; + + /* + The following keycodes have been shown not to work as accelerator + keys on GTK (see https://trac.wxwidgets.org/ticket/10049) + and are not valid + (see gtkaccelgroup.c inside gtk_accelerator_valid()) + */ + case WXK_COMMAND: // Same as WXK_CONTROL + case WXK_SHIFT: + case WXK_ALT: + case WXK_SCROLL: // Scroll lock + case WXK_CAPITAL: // Caps lock + case WXK_NUMLOCK: + case WXK_NUMPAD_TAB: + case WXK_TAB: + case WXK_WINDOWS_LEFT: + case WXK_WINDOWS_RIGHT: + + /* + The following keycodes do not map clearly into a GTK keycode, + so they are not included in the accelerator mapping: + */ + case WXK_ADD: + case WXK_SEPARATOR: + case WXK_SUBTRACT: + case WXK_DECIMAL: + case WXK_DIVIDE: + case WXK_SNAPSHOT: + + /* + The following special codes do not map into GTK keycodes, + see gdk/keynames.txt + */ + case WXK_SPECIAL1: case WXK_SPECIAL2: case WXK_SPECIAL3: + case WXK_SPECIAL4: case WXK_SPECIAL5: case WXK_SPECIAL6: + case WXK_SPECIAL7: case WXK_SPECIAL8: case WXK_SPECIAL9: + case WXK_SPECIAL10: case WXK_SPECIAL11: case WXK_SPECIAL12: + case WXK_SPECIAL13: case WXK_SPECIAL14: case WXK_SPECIAL15: + case WXK_SPECIAL16: case WXK_SPECIAL17: case WXK_SPECIAL18: + case WXK_SPECIAL19: case WXK_SPECIAL20: + valid = false; + wxLogDebug( wxString::Format( wxT("Compatibility issue: %s is not supported as a keyboard accelerator key on GTK"), + keyname ) ); + break; + } + + return valid; +} + /* static */ bool wxAcceleratorEntry::ParseAccel(const wxString& text, int *flagsOut, int *keyOut) From 2c146d830d6b5b50f7440c423eee4661168774d9 Mon Sep 17 00:00:00 2001 From: Ian McInerney Date: Sun, 25 Aug 2019 17:31:02 +0200 Subject: [PATCH 3/3] Fixes from review --- src/common/accelcmn.cpp | 8 +++---- src/gtk/menu.cpp | 49 +++++++++-------------------------------- 2 files changed, 15 insertions(+), 42 deletions(-) diff --git a/src/common/accelcmn.cpp b/src/common/accelcmn.cpp index bc8937a2b8..6bfb3a184c 100644 --- a/src/common/accelcmn.cpp +++ b/src/common/accelcmn.cpp @@ -193,8 +193,8 @@ bool wxAcceleratorEntry::ValidateKey(int flags, int keycode) if ( !flags ) { valid = false; - wxLogDebug( wxString::Format( wxT("Compatibility issue: %s key must have modifiers to be an accelerator key on GTK"), - keyname ) ); + wxLogDebug( "Compatibility issue: %s key must have modifiers to be an accelerator key on GTK", + keyname ); } break; @@ -238,8 +238,8 @@ bool wxAcceleratorEntry::ValidateKey(int flags, int keycode) case WXK_SPECIAL16: case WXK_SPECIAL17: case WXK_SPECIAL18: case WXK_SPECIAL19: case WXK_SPECIAL20: valid = false; - wxLogDebug( wxString::Format( wxT("Compatibility issue: %s is not supported as a keyboard accelerator key on GTK"), - keyname ) ); + wxLogDebug( "Compatibility issue: %s is not supported as a keyboard accelerator key on GTK", + keyname ); break; } diff --git a/src/gtk/menu.cpp b/src/gtk/menu.cpp index 3065fa2564..05d0bc0326 100644 --- a/src/gtk/menu.cpp +++ b/src/gtk/menu.cpp @@ -1062,37 +1062,25 @@ static wxString GetGtkHotKey( const wxMenuItem& item ) if ( flags ) hotkey << wxT("Up" ); else - { - wxLogDebug( wxT("The Up key must have modifiers to be an accelerator key") ); - hotkey.Clear(); - } + hotkey.clear(); break; case WXK_DOWN: if ( flags ) hotkey << wxT("Down" ); else - { - wxLogDebug( wxT("The Down key must have modifiers to be an accelerator key") ); - hotkey.Clear(); - } + hotkey.clear(); break; case WXK_LEFT: if ( flags ) hotkey << wxT("Left" ); else - { - wxLogDebug( wxT("The Left key must have modifiers to be an accelerator key") ); - hotkey.Clear(); - } + hotkey.clear(); break; case WXK_RIGHT: if ( flags ) hotkey << wxT("Right" ); else - { - wxLogDebug( wxT("The Right key must have modifiers to be an accelerator key") ); - hotkey.Clear(); - } + hotkey.clear(); break; case WXK_PAGEUP: hotkey << wxT("Page_Up" ); @@ -1168,37 +1156,25 @@ static wxString GetGtkHotKey( const wxMenuItem& item ) if ( flags ) hotkey << wxT("KP_Up" ); else - { - wxLogDebug( wxT("The KP_Up key must have modifiers to be an accelerator key") ); - hotkey.Clear(); - } + hotkey.clear(); break; case WXK_NUMPAD_DOWN: if ( flags ) hotkey << wxT("KP_Down" ); else - { - wxLogDebug( wxT("The KP_Down key must have modifiers to be an accelerator key") ); - hotkey.Clear(); - } + hotkey.clear(); break; case WXK_NUMPAD_LEFT: if ( flags ) hotkey << wxT("KP_Left" ); else - { - wxLogDebug( wxT("The KP_Left key must have modifiers to be an accelerator key") ); - hotkey.Clear(); - } + hotkey.clear(); break; case WXK_NUMPAD_RIGHT: if ( flags ) hotkey << wxT("KP_Right" ); else - { - wxLogDebug( wxT("The KP_Right key must have modifiers to be an accelerator key") ); - hotkey.Clear(); - } + hotkey.clear(); break; case WXK_NUMPAD_PAGEUP: hotkey << wxT("KP_Page_Up" ); @@ -1282,9 +1258,7 @@ static wxString GetGtkHotKey( const wxMenuItem& item ) case WXK_SPECIAL13: case WXK_SPECIAL14: case WXK_SPECIAL15: case WXK_SPECIAL16: case WXK_SPECIAL17: case WXK_SPECIAL18: case WXK_SPECIAL19: case WXK_SPECIAL20: - wxLogDebug( wxString::Format( wxT("Unsupported keyboard accelerator key: %s"), - accel->ToString() ) ); - hotkey.Clear(); + hotkey.clear(); break; // if there are any other keys wxAcceleratorEntry::Create() may @@ -1302,9 +1276,8 @@ static wxString GetGtkHotKey( const wxMenuItem& item ) } } - wxLogDebug( wxString::Format( wxT("unknown keyboard accelerator key code: %i"), - code ) ); - hotkey.Clear(); + wxLogDebug( "Unknown keyboard accelerator key code: %i", code ); + hotkey.clear(); } delete accel;