Tab accelerator key isn't supported by gtk

[carrotIndustries]

System information

SolveSpace version: git master 0da4a6b

Operating system: Arch Linux, X11

Expected behavior

Pressing Tab should toggle the property browser

Actual behavior

Nothing happens. This is due to Gtk not considering Tab to to be a valid accelerator: https://gitlab.gnome.org/GNOME/gtk/-/blob/gtk-3-24/gtk/gtkaccelgroup.c#L982

Suggested fixes

• Use a different default accelerator
• Roll our own gtk_accel_groups_activate and hook it up to the appropriate signal in GtkWindow or so to work around gtk_accelerator_valid

[whitequark]

I'm confused. That file hasn't changed in many years, it used to work fine in 2016, and it works just fine now on my machine. How did it break?

[whitequark]

More to the topic, we already handle a few not-quite-accelerators in OnKeyPress or a related event; Tab could go there too.

[whitequark]

Actually, we already do that in

solvespace/src/graphicswin.cpp

Lines 253 to 260 in 0da4a6b

	// On some platforms, the OS does not handle some or all keyboard accelerators,
	// so handle them here.
	for(int i = 0; Menu[i].level >= 0; i++) {
	if(AcceleratorForCommand(Menu[i].cmd).Equals(event)) {
	ActivateCommand(Menu[i].cmd);
	return true;
	}
	}

You're going to have to figure out why that code path is never reached, since Tab works fine for me locally.

Nothing happens. This is due to Gtk not considering Tab to to be a valid accelerator:

• OS: Arch Linux, X11,
• GTK: 3.24

JFTR, Strange, because toggling "Property Browser" via Tab key works for me:

• OS: MX Linux MX-19.1 (Debian 10 buster), Xfce4
• GTK: 3.24.5

[carrotIndustries]

You're going to have to figure out why that code path is never reached, since Tab works fine for me locally.

Good to know that there's another code path.

solvespace/src/platform/guigtk.cpp

Lines 568 to 570 in 0da4a6b

	if(gdk_event->state & ~(GDK_SHIFT_MASK|GDK_CONTROL_MASK)) {
	return false;
	}

On my system apparently, state is GDK_MOD2_MASK even with no modifier pressed. Since the above code exits early if a modifier other than shift or ctrl is present GraphicsWindow::KeyboardEvent never gets called. Removing this check makes tab behave as intended.

[whitequark]

On my system apparently, state is GDK_MOD2_MASK even with no modifier pressed.

Why does this happen?

[carrotIndustries]

Why does this happen?

On my system GDK_MOD2_MASK is set if Num Lock is on. If num lock is off the existing code works as intended.

As far as I can tell we can therefore safely ignore MOD2.

[whitequark]

Does this help?

diff --git a/src/platform/guigtk.cpp b/src/platform/guigtk.cpp
index e68fd304..461ba60b 100644
--- a/src/platform/guigtk.cpp
+++ b/src/platform/guigtk.cpp
@@ -565,7 +565,7 @@ protected:
         KeyboardEvent event = {};
         event.type = type;
 
-        if(gdk_event->state & ~(GDK_SHIFT_MASK|GDK_CONTROL_MASK)) {
+        if(gdk_event->state & GDK_MOD1_MASK) {
             return false;
         }
 

[whitequark]

Sorry, the earlier comment had a typo--I meant to use GDK_MOD1_MASK (fixed now).

[carrotIndustries]

Depends on what we want to achieve:

• Ignore MOD1 (Alt) specifically
• Ignore all unknown modifiers such as super

From my point of view, we want the latter option since Super-tab shouldn't trigger our accelerator:

diff --git a/src/platform/guigtk.cpp b/src/platform/guigtk.cpp
index e68fd30..fe31466 100644
--- a/src/platform/guigtk.cpp
+++ b/src/platform/guigtk.cpp
@@ -565,7 +565,7 @@ protected:
         KeyboardEvent event = {};
         event.type = type;
 
-        if(gdk_event->state & ~(GDK_SHIFT_MASK|GDK_CONTROL_MASK)) {
+        if(gdk_event->state & ~(GDK_SHIFT_MASK|GDK_CONTROL_MASK|GDK_MOD2_MASK)) {
             return false;
         }

[whitequark]

What other bugs remain in that code? Is there the same issue with ScrollLock and CapsLock?

[carrotIndustries]

I had a closer look at the Gtk docs and I think I found what we need:

diff --git a/src/platform/guigtk.cpp b/src/platform/guigtk.cpp
index e68fd30..7ff0478 100644
--- a/src/platform/guigtk.cpp
+++ b/src/platform/guigtk.cpp
@@ -565,7 +565,8 @@ protected:
         KeyboardEvent event = {};
         event.type = type;
 
-        if(gdk_event->state & ~(GDK_SHIFT_MASK|GDK_CONTROL_MASK)) {
+        const auto mod_mask = get_modifier_mask(Gdk::MODIFIER_INTENT_DEFAULT_MOD_MASK);
+        if((gdk_event->state & mod_mask) & ~(GDK_SHIFT_MASK|GDK_CONTROL_MASK)) {
             return false;
         }

https://developer.gnome.org/gdk3/stable/gdk3-Windows.html#GdkModifierIntent

The set of modifier masks accepted as modifiers in accelerators. Needed because Command is mapped to MOD2 on OSX, which is widely used, but on X11 MOD2 is NumLock and using that for a mod key is problematic at best.

[whitequark]

Thanks!