From 68b36aed6d50f9ee697c4e5b7220e5e5c40e3f2f Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Thu, 26 Sep 2019 23:13:14 +0200 Subject: [PATCH] Only give messages about unsupported accelerators in wxGTK Doing this under all platforms results in too many false positives, which can't be avoided currently, e.g. even if an application uses "Tab" as an accelerator only under MSW, these messages still appear (in debug builds, but this is more than sufficient for them to be annoying). For now, restrict the messages to wxGTK only. In the future we could revert to giving them under all platforms if we provide some way of disabling them, e.g. qualifying accelerators with "[port]" or "[!port]" string before them. This partially reverts 6596f5a98d15d49e217ab50b23461003f77ee513, see https://github.com/wxWidgets/wxWidgets/pull/1505 Closes https://github.com/wxWidgets/wxWidgets/pull/1566 --- include/wx/accel.h | 8 +-- src/common/accelcmn.cpp | 90 ------------------------------ src/gtk/menu.cpp | 119 ++++++++++++++++++++++++++-------------- 3 files changed, 79 insertions(+), 138 deletions(-) diff --git a/include/wx/accel.h b/include/wx/accel.h index c0f23ca534..c1f78d58ed 100644 --- a/include/wx/accel.h +++ b/include/wx/accel.h @@ -53,9 +53,7 @@ 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 @@ -63,8 +61,6 @@ public: void Set(int flags, int keyCode, int cmd, wxMenuItem *item = NULL) { - ValidateKey(flags, keyCode); - m_flags = flags; m_keyCode = keyCode; m_command = cmd; @@ -122,8 +118,6 @@ 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 6bfb3a184c..820985df26 100644 --- a/src/common/accelcmn.cpp +++ b/src/common/accelcmn.cpp @@ -156,96 +156,6 @@ 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( "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( "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) diff --git a/src/gtk/menu.cpp b/src/gtk/menu.cpp index 89996ee00d..89976006ae 100644 --- a/src/gtk/menu.cpp +++ b/src/gtk/menu.cpp @@ -35,7 +35,7 @@ extern int wxOpenModalDialogsCount; static const int wxGTK_TITLE_ID = -3; #if wxUSE_ACCEL -static void wxGetGtkAccel(const wxMenuItem*, guint*, GdkModifierType*); +static bool wxGetGtkAccel(const wxMenuItem*, guint*, GdkModifierType*); #endif // Unity hack: under Ubuntu Unity the global menu bar is not affected by a @@ -643,8 +643,7 @@ void wxMenuItem::SetItemLabel( const wxString& str ) // remove old accelerator guint accel_key; GdkModifierType accel_mods; - wxGetGtkAccel(this, &accel_key, &accel_mods); - if (accel_key) + if ( wxGetGtkAccel(this, &accel_key, &accel_mods) ) { gtk_widget_remove_accelerator( m_menuItem, GetRootParentMenu(m_parentMenu)->m_accel, accel_key, accel_mods); @@ -664,8 +663,7 @@ void wxMenuItem::SetGtkLabel() #if wxUSE_ACCEL guint accel_key; GdkModifierType accel_mods; - wxGetGtkAccel(this, &accel_key, &accel_mods); - if ( accel_key && gtk_accelerator_valid( accel_key, accel_mods ) ) + if ( wxGetGtkAccel(this, &accel_key, &accel_mods) ) { gtk_widget_add_accelerator( m_menuItem, "activate", GetRootParentMenu(m_parentMenu)->m_accel, @@ -1038,6 +1036,18 @@ static wxString GetGtkHotKey( const wxMenuItem& item ) if ( flags & wxACCEL_SHIFT ) hotkey += wxT(""); + // Accelerator can be invalid for 2 different reasons from GTK point of + // view: either the key just can't be used as an accelerator at all + // (e.g. TAB), or it can only be used as an accelerator with modifiers. + // This variable can be set to one of the matching values in the switch + // below and is used to give the most fitting error message later. + enum Validity + { + Valid_Always, + Valid_Never, + Valid_WithModifiers + } validity = Valid_Always; + int code = accel->GetKeyCode(); switch ( code ) { @@ -1059,28 +1069,20 @@ static wxString GetGtkHotKey( const wxMenuItem& item ) hotkey << wxT("Delete" ); break; case WXK_UP: - if ( flags ) - hotkey << wxT("Up" ); - else - hotkey.clear(); + validity = Valid_WithModifiers; + hotkey << wxT("Up" ); break; case WXK_DOWN: - if ( flags ) - hotkey << wxT("Down" ); - else - hotkey.clear(); + validity = Valid_WithModifiers; + hotkey << wxT("Down" ); break; case WXK_LEFT: - if ( flags ) - hotkey << wxT("Left" ); - else - hotkey.clear(); + validity = Valid_WithModifiers; + hotkey << wxT("Left" ); break; case WXK_RIGHT: - if ( flags ) - hotkey << wxT("Right" ); - else - hotkey.clear(); + validity = Valid_WithModifiers; + hotkey << wxT("Right" ); break; case WXK_PAGEUP: hotkey << wxT("Page_Up" ); @@ -1153,28 +1155,20 @@ static wxString GetGtkHotKey( const wxMenuItem& item ) hotkey << wxT("KP_Home" ); break; case WXK_NUMPAD_UP: - if ( flags ) - hotkey << wxT("KP_Up" ); - else - hotkey.clear(); + validity = Valid_WithModifiers; + hotkey << wxT("KP_Up" ); break; case WXK_NUMPAD_DOWN: - if ( flags ) - hotkey << wxT("KP_Down" ); - else - hotkey.clear(); + validity = Valid_WithModifiers; + hotkey << wxT("KP_Down" ); break; case WXK_NUMPAD_LEFT: - if ( flags ) - hotkey << wxT("KP_Left" ); - else - hotkey.clear(); + validity = Valid_WithModifiers; + hotkey << wxT("KP_Left" ); break; case WXK_NUMPAD_RIGHT: - if ( flags ) - hotkey << wxT("KP_Right" ); - else - hotkey.clear(); + validity = Valid_WithModifiers; + hotkey << wxT("KP_Right" ); break; case WXK_NUMPAD_PAGEUP: hotkey << wxT("KP_Page_Up" ); @@ -1258,7 +1252,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: - hotkey.clear(); + validity = Valid_Never; break; // if there are any other keys wxAcceleratorEntry::Create() may @@ -1280,33 +1274,76 @@ static wxString GetGtkHotKey( const wxMenuItem& item ) hotkey.clear(); } + switch ( validity ) + { + case Valid_Always: + break; + + case Valid_Never: + wxLogDebug("\"%s\" is not supported as " + "a keyboard accelerator with GTK", + accel->ToString()); + hotkey.clear(); + break; + + case Valid_WithModifiers: + if ( !flags ) + { + wxLogDebug("\"%s\" must use modifiers to be used as " + "a keyboard accelerator with GTK", + accel->ToString()); + hotkey.clear(); + } + break; + } + delete accel; } return hotkey; } -static void +static bool wxGetGtkAccel(const wxMenuItem* item, guint* accel_key, GdkModifierType* accel_mods) { - *accel_key = 0; const wxString string = GetGtkHotKey(*item); if (!string.empty()) + { gtk_accelerator_parse(wxGTK_CONV_SYS(string), accel_key, accel_mods); + + // Normally, we detect all the keys considered invalid by GTK in + // GetGtkHotKey(), but just in case GTK decides to add more invalid + // keys in the future versions, recheck once again using its function. + if ( gtk_accelerator_valid(*accel_key, *accel_mods) ) + return true; + + wxLogDebug("\"%s\" is not a valid keyboard accelerator " + "for this GTK version", + string); + } #ifndef __WXGTK4__ else { wxGCC_WARNING_SUPPRESS(deprecated-declarations) + + // Check if this is a stock item for which GTK defines a default + // accelerator. GtkStockItem stock_item; const char* stockid = wxGetStockGtkID(item->GetId()); - if (stockid && gtk_stock_lookup(stockid, &stock_item)) + if ( stockid && + gtk_stock_lookup(stockid, &stock_item) && + stock_item.keyval ) { *accel_key = stock_item.keyval; *accel_mods = stock_item.modifier; + + return true; } wxGCC_WARNING_RESTORE() } #endif + + return false; } #endif // wxUSE_ACCEL