After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 792978 - Mnemonics don't work in prefs editor
Mnemonics don't work in prefs editor
Status: RESOLVED FIXED
Product: gnome-terminal
Classification: Core
Component: Profiles
git master
Other Linux
: Normal minor
: ---
Assigned To: GNOME Terminal Maintainers
GNOME Terminal Maintainers
Depends on:
Blocks:
 
 
Reported: 2018-01-27 19:55 UTC by Egmont Koblinger
Modified: 2018-11-06 14:57 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Fix (604 bytes, patch)
2018-01-29 21:19 UTC, Egmont Koblinger
none Details | Review
slightly hacky poc patch (3.10 KB, patch)
2018-01-30 17:55 UTC, Christian Persch
none Details | Review
unity poc, doesn't work (11.12 KB, patch)
2018-01-31 21:50 UTC, Egmont Koblinger
none Details | Review
Both approaches in one, v1 (11.50 KB, patch)
2018-02-02 21:07 UTC, Egmont Koblinger
none Details | Review
Both approaches in one, v2 (11.36 KB, patch)
2018-02-03 16:42 UTC, Egmont Koblinger
none Details | Review
next try of a gtk patch (conceptual, not even compile tested) (3.18 KB, patch)
2018-02-03 18:04 UTC, Christian Persch
none Details | Review
Both approaches in one, v3 (11.69 KB, patch)
2018-02-04 00:28 UTC, Egmont Koblinger
none Details | Review
Both approaches in one, v4 (12.90 KB, patch)
2018-02-04 19:58 UTC, Egmont Koblinger
committed Details | Review

Description Egmont Koblinger 2018-01-27 19:55:56 UTC
Mnemonics don't show up in the preferences editor.

Long-press Alt -> Nothing happens. Or figure out the hotkey by looking at the source, and then press Alt + hotkey -> no action.

Interestingly, they do work if gnome-terminal is not yet running and I start "gnome-terminal --preferences".

(I've thought for a long time that it's something misconfigured on my system, but now I start to have a feeling it's really gnome-terminal's fault.)
Comment 1 Egmont Koblinger 2018-01-27 22:36:26 UTC
So, it's related to the "Enable mnemonics" config option. If enabled there, they work everywhere.

terminal_window_screen_update() goes like

  gtk_settings = gtk_settings_get_for_screen (screen);
  [...]
  g_settings_bind (settings,
                   TERMINAL_SETTING_ENABLE_MNEMONICS_KEY,
                   gtk_settings,
                   "gtk-enable-mnemonics",
                   G_SETTINGS_BIND_GET);

which I suspect is skipped when opening the prefs window directly, whereas gtk_settings_get_for_screen() is way too global and applies to the prefs window too. We should find a replacement that only applies to the terminal windows.
Comment 2 Egmont Koblinger 2018-01-27 22:58:48 UTC
GtkSettings seems to be per-screen, with no way to make it per-window. Unless I'm missing something, of course.

A workaround I can think of is to toggle this setting on focus-in events of terminal vs. prefs window.
Comment 3 Christian Persch 2018-01-28 21:47:45 UTC
Yes, the GtkSettings is global per screen, and so the setting must be done on the single instance.

The setting must be set to false so that the menubar doesn't grab Alt+Letter keys, and there's no other way to do that that I can see in the gtk+ code :-((

Toggling on focus in/out would be way too hacky IMHO.

The least-hacky way I can think of would be to do what the gtk inspector does and put the prefs dialogue on a new GdkDisplay (which then has its own GtkSettings) copy of the default display, i.e. gdk_display_open(NULL).
Comment 4 Egmont Koblinger 2018-01-29 21:19:27 UTC
Created attachment 367607 [details] [review]
Fix

> The least-hacky way I can think of would be to do what the gtk inspector does
> and put the prefs dialogue on a new GdkDisplay (which then has its own
> GtkSettings) copy of the default display, i.e. gdk_display_open(NULL).

Yup, it's actually as simple as this, and works for me (tested with both unity7 and gnome-shell).

Patch goes on top of prefs-merge.
Comment 5 Egmont Koblinger 2018-01-30 09:00:26 UTC
... and the same should be done for the Find dialog too. Which works as long as it's a separate dialog and not a popover.

There are further bugs with our current approach:

- If you invoke the menu using F10, you don't get mnemonics either, although they would be useful for further navigation.

- There are no mnemonics in the right-click context menu either, which can also be invoked using the "menu" button present on most keyboards.

---

I believe the only place where the presence of mnemonics is potentially harmful is the main row of the application menu (File Edit ... Help). Everywhere else they are harmless, is this true?

How about turning this whole story inside out?

Let's not fiddle with gtk-enable-mnemonics at all. Instead, simply (optionally) install the application menu so that its topmost entries just don't have mnemonics ("File" instead of "_File", etc.).

(This would also work around bug 792312 comment 11.)
Comment 6 Egmont Koblinger 2018-01-30 10:02:41 UTC
Is it just me, or is there really no way to alter the label attribute of a [sub]menu (rather than an item)?
Comment 7 Egmont Koblinger 2018-01-30 11:09:40 UTC
Assuming that indeed the label cannot be modified... I still think that despite this local ugliness, this approach is better (more user friendly) than our current one.

There's two things to solve:

1. Dynamically install a brand new entire menu when the setting changes. I'm pretty sure it's doable.

(Worst case, if not, we'll have a config option that requires application restart. IMO still a better user experience than our current one, as this one would have a drawback when changing a pref, while our current one has drawbacks during everyday usage.)

2. Create this menu based on the setting.

One approach could be to build up the menu manually in C source, rather than from a .ui file. A bit cumbersome but probably not that terrible.

Another approach could be to maintain two versions in parallel manually, hell no.

Yet another approach could be to do some preprocessing. Here I have two ideas:

a. Placeholders like
  <attribute name="label">%%FILE%%</attribute>
Need to be substituted runtime: load the .ui file into a string, search-replace, and then initialize the Builder from that string.

b. ifdef-style xml tags, like
  <WITH_MNEMONIC><attribute name="label">_File</attribute></WITH_MNEMONIC>
  <WITHOUT_MNEMONIC><attribute name="label">File</attribute></WITHOUT_MNEMONIC>
For one pair, search-replace the tags only to the empty string. For the other pair, search-replace the tags and what's in between to the empty string using a simple regex, or search-replace just the tags themselves to xml comment tags "<!-- " and " -->". The .ui file is valid xml and translations can be extracted as usual, and it's a bit more translator-friendly in the sense that top menu labels and submenu labels get close to each other. Can be search-replaced either at build time or runtime, whichever is more convenient for us.
Comment 8 Christian Persch 2018-01-30 12:58:39 UTC
All these possibilities look too complicate to me.

Let's go back and see why we're using this gtksetting at all. The problem is that if we do not, then every Alt+Letter key press activates the menu instead of being available to be sent to the terminal. The code doing that is in gtk+/gtk/gtkmenushell.c:gtk_menu_shell_key_press(). And then GtkMenuBar derives from GtkMenuShell, so inherits this handler.

What we could try is to:
* Don't set the gtk-enable-mnemonics setting.

* Make a TerminalMenuBar derived from GtkMenuBar, that overrides the key_press handler and only chains up to the parent handler if the menubar has the focus.

* Use that TerminalMenuBar in TerminalWindow; instead of gtk_menu_bar_new_from_model(), create our own one and use gtk_menu_shell_bind_model() on it.
Comment 9 Egmont Koblinger 2018-01-30 15:32:52 UTC
(In reply to Christian Persch from comment #8)

> * Make a TerminalMenuBar derived from GtkMenuBar

Would this approach also work with Unity's out-of-the-window menubar?
Comment 10 Christian Persch 2018-01-30 17:55:07 UTC
I found another way which is even lighter, and it works— except that despite mnemonics not being activated, they still light up on Alt key press until key release. This is due to a bug in gtk+/gtk/gtkmain.c:gtk_main_do_event(), which hardcodes Alt for this instead of consulting the window's mnemonic modifier:

/* Catch alt press to enable auto-mnemonics;
 * menus are handled elsewhere
 * FIXME: this does not work with mnemonic modifiers other than Alt
 */
if ((event->key.keyval == GDK_KEY_Alt_L || event->key.keyval == GDK_KEY_Alt_R) &&
    ((event->key.state & (gtk_accelerator_get_default_mod_mask ()) & ~(GDK_RELEASE_MASK|GDK_MOD1_MASK)) == 0) &&
    !GTK_IS_MENU_SHELL (grab_widget))

Duh!

Changing that to something like

guint mnemonic_modifier = gtk_window_get_mnemonic_modifier(window);

if (((event->key.state & mnemonic_modifier) == mnemonic_modifier) &&
    ((event->key.state & ~(GDK_RELEASE_MASK | mnemonic_modifier)) == 0 &&
    !GTK_IS_MENU_SHELL (grab_widget))

*should* work, but I'm not doing a long gtk recompile just to test :-)

If someone could try that and convince upstream to fix it, that would be great :-)
Comment 11 Christian Persch 2018-01-30 17:55:47 UTC
Created attachment 367654 [details] [review]
slightly hacky poc patch
Comment 12 Christian Persch 2018-01-30 17:57:02 UTC
(In reply to Egmont Koblinger from comment #9)
> (In reply to Christian Persch from comment #8)
> Would this approach also work with Unity's out-of-the-window menubar?

I don't think so since AFAIK it's out of process, right?; and neither would the patch above.

I could take a look at their code; do you know where this menubar code lives?
Comment 13 Egmont Koblinger 2018-01-30 20:02:19 UTC
(In reply to Christian Persch from comment #12)

> > Would this approach also work with Unity's out-of-the-window menubar?
> 
> I don't think so since AFAIK it's out of process, right?; and neither would
> the patch above.

That's what I was afraid of. I'll test it (and also your GTK+ patch).

> I could take a look at their code; do you know where this menubar code lives?

No idea at all, sorry, I just happen to prefer Unity as an end user, that's all I know about it.


Conceptually I'd still prefer the approach that doesn't install a mnemonic, rather than hacks around not to activate on it. Plus I'm somewhat worried that in forthcoming Ubuntu LTS this would be a usability regression with no workaround (other than re-training muscle memory for Esc + F etc.) for some of the old users (those who stick with the former default Unity).

I'd rather have a somewhat uglier source implementing the IMHO better global design and better user experience, than the other way around.


>     gtk_window_set_mnemonic_modifier (GTK_WINDOW (window), GDK_MODIFIER_MASK);

Actually, having Alt+Shift as the modifier, in spirit of Ctrl+Shift+C and friends, might also be another direction to think about. (The future Find popover issue might still persist, though... wait, I guess we could easily toggle the modifier back-n-forth when this popover is presented, correct?)


Maybe take another few steps back... Do we want to retain this option at all? Couldn't we maybe compromise on Alt+Shift and say that it should be good enough for everyone? Or think about other ways of invoking the menu, e.g. the context menu (which can be opened using the Menu key, which doesn't interfere with terminal emulation) could contain an entry to focus the global menu...??
Comment 14 Christian Persch 2018-01-30 21:00:55 UTC
(In reply to Egmont Koblinger from comment #13)
> Conceptually I'd still prefer the approach that doesn't install a mnemonic,
> rather than hacks around not to activate on it. Plus I'm somewhat worried
> that in forthcoming Ubuntu LTS this would be a usability regression with no
> workaround (other than re-training muscle memory for Esc + F etc.) for some
> of the old users (those who stick with the former default Unity).

The approach above should fix the non-unity case; let's see how to fix the unity case.
 
(In reply to Egmont Koblinger from comment #7)
> 1. Dynamically install a brand new entire menu when the setting changes. I'm
> pretty sure it's doable.

The setting doesn't change, or if it does, we shoudn't care.
 
> (Worst case, if not, we'll have a config option that requires application
> restart. IMO still a better user experience than our current one, as this
> one would have a drawback when changing a pref, while our current one has
> drawbacks during everyday usage.)
> 
> 2. Create this menu based on the setting.

So, we don't actually use the GtkApplication::menubar to build our in-window menus. We load the UI from file, and only set the menu model in the application if the setting is true. So we could do is. Depending on the setting, load 2 different menu files, one with and one without the toplevel accels.

> One approach could be to build up the menu manually in C source, rather than
> from a .ui file. A bit cumbersome but probably not that terrible.

Urgh, no. Very terrible :-)

> Another approach could be to maintain two versions in parallel manually,
> hell no.

No.
 
> Yet another approach could be to do some preprocessing. Here I have two
> ideas:

Yes, preprocessing is the way to go. A simple script should be able to create the no-toplevel-mnemonics one from the stock one at build time; the gresources would contain both versions and we'll load the right one.

> a. Placeholders like
>   <attribute name="label">%%FILE%%</attribute>
> Need to be substituted runtime: load the .ui file into a string,
> search-replace, and then initialize the Builder from that string.

No no no :-)
 
> b. ifdef-style xml tags, like
>   <WITH_MNEMONIC><attribute name="label">_File</attribute></WITH_MNEMONIC>
>   <WITHOUT_MNEMONIC><attribute
> name="label">File</attribute></WITHOUT_MNEMONIC>
> For one pair, search-replace the tags only to the empty string. For the
> other pair, search-replace the tags and what's in between to the empty
> string using a simple regex, or search-replace just the tags themselves to
> xml comment tags "<!-- " and " -->". The .ui file is valid xml and
> translations can be extracted as usual, and it's a bit more
> translator-friendly in the sense that top menu labels and submenu labels get
> close to each other. Can be search-replaced either at build time or runtime,
> whichever is more convenient for us.

A perl or python script should do the trick. Select <submenu> direct child of <menu id="menubar">, select <attribue name="label"> therein, and remove the mnemonic from the contents.

Or use xsltproc :-)

Or in a pinch, do work manually once (copy .ui file, modify toplevel labels), create patch with diff(1) and commit that patch and Makefile.am does copy & patch.

The last one is probably the simplest one, and still maintainable.
Comment 15 Egmont Koblinger 2018-01-30 21:16:07 UTC
> The approach above should fix the non-unity case; let's see how to fix the
> unity case.

If fixing the unity case fixes both then there's no need for the non-unity fix :)

> The setting doesn't change, or if it does, we shoudn't care.

I mean if the user toggles that preference. This would be the only one that requires app restart. Anyway, save it for the second round.

> A perl or python script should do the trick. Select <submenu> direct child
> of <menu id="menubar">, select <attribue name="label"> therein, and remove
> the mnemonic from the contents.

As per bug 757546, no automatic removal of the underscore, it should be done by translators.

> Or use xsltproc :-)

If preprocessing at build time, I'd go for the simplest possible approach and wouldn't mind at all relying on a specific formatting of the xml. Maybe:

<!-- WITH_MNE --> <attribute name="label" translatable="yes">_File</attribute>
<!-- WITHOUT_MNE --><attribute name="label" translatable="yes">File</attribute>

and then a preprocessing as simple as a "grep -v". I really don't want to fiddle with proper handling of xml.

> create patch with diff(1) and commit that patch and Makefile.am
> does copy & patch.

I don't really like this one. :-P
Comment 16 Egmont Koblinger 2018-01-30 21:23:32 UTC
(In reply to Christian Persch from comment #10)

> guint mnemonic_modifier = gtk_window_get_mnemonic_modifier(window);

[window goes out of scope just before this]

> if (((event->key.state & mnemonic_modifier) == mnemonic_modifier) &&
>     ((event->key.state & ~(GDK_RELEASE_MASK | mnemonic_modifier)) == 0 &&
>     !GTK_IS_MENU_SHELL (grab_widget))

[unbalanced parens]

Anyway, after fixing the trivial errors it still doesn't work. Mnemonics aren't underlined at either setting. I might further investigate later (but as said, I'm not too keen on this approach).

> > Would this approach also work with Unity's out-of-the-window menubar?

> I don't think so since AFAIK it's out of process, right?; and neither
> would the patch above.

Obviously, as expected, this patch has no effect on Unity.
Comment 17 Egmont Koblinger 2018-01-30 21:27:39 UTC
(In reply to Egmont Koblinger from comment #15)

> > and remove the mnemonic from the contents.
> 
> As per bug 757546, no automatic removal of the underscore, it should be done
> by translators.

Another problem: then only the mnemonic version would be sent for translation, and then the non-mnemonic one would be looked up which wouldn't be found in the dictionary. So no, both versions of the strings have to be present checked in to our source tree, before any preprocessing takes place.

> create patch with diff(1) and commit that patch

Ditto, wouldn't work.
Comment 18 Christian Persch 2018-01-30 21:35:57 UTC
(In reply to Egmont Koblinger from comment #15)
> > The approach above should fix the non-unity case; let's see how to fix the
> > unity case.
> 
> If fixing the unity case fixes both then there's no need for the non-unity
> fix :)

Well, the change outlined above together with the gtk fix (if it works) is rather simpler than rebuilding the whole menu.

> > The setting doesn't change, or if it does, we shoudn't care.
> 
> I mean if the user toggles that preference. This would be the only one that
> requires app restart. Anyway, save it for the second round.

I meant the gtk-shell-shows-menu-bar setting.

The other pref must of course be obeyed since we have UI for it.

> > A perl or python script should do the trick. Select <submenu> direct child
> > of <menu id="menubar">, select <attribue name="label"> therein, and remove
> > the mnemonic from the contents.
> 
> As per bug 757546, no automatic removal of the underscore, it should be done
> by translators.

This is different. The automatic removal applies to the *source* string, ie the *un*translated string. So we know it only has one underscore.

Of course this means we need to add the extra .ui file to POTFILES.in, and that means it must be in the repo, not generated at build time...
 
(In reply to Egmont Koblinger from comment #16)
> (In reply to Christian Persch from comment #10)
> 
> > guint mnemonic_modifier = gtk_window_get_mnemonic_modifier(window);
> 
> [window goes out of scope just before this]

Need to extend the scope obviously.

> > if (((event->key.state & mnemonic_modifier) == mnemonic_modifier) &&
> >     ((event->key.state & ~(GDK_RELEASE_MASK | mnemonic_modifier)) == 0 &&
> >     !GTK_IS_MENU_SHELL (grab_widget))
> 
> [unbalanced parens]

Right that was just editing in the comment field here, not compile or anything tested.

> Anyway, after fixing the trivial errors it still doesn't work. Mnemonics
> aren't underlined at either setting. I might further investigate later (but
> as said, I'm not too keen on this approach).

If you have a bit more time, could you what bits are set in event->key.state, and on which of the conditionals it bails in the long if() it bails out?
Comment 19 Egmont Koblinger 2018-01-30 21:51:54 UTC
(In reply to Christian Persch from comment #18)

> Well, the change outlined above together with the gtk fix (if it works) is
> rather simpler than rebuilding the whole menu.

I'm afraid I missed out on a few details here (around GtkApplication::menubar). Is altering the menu significantly simpler for Unity (e.g. need to change only the model) than GNOME Shell (need to change the entire GTK+ widgets representing the menu)? If so then I'm fine with this direction.

> This is different. The automatic removal applies to the *source* string, ie
> the *un*translated string. So we know it only has one underscore.

Indeed.

> Of course this means we need to add the extra .ui file to POTFILES.in, and
> that means it must be in the repo, not generated at build time...

I don't fancy the concept of autogenerated _and_ checked-in files, IMO it should be either-or. We used to have it... where exactly, maybe at box drawing, or vteseq? can't recall, and it caused some troubles.

> If you have a bit more time, could you what bits are set in
> event->key.state, and on which of the conditionals it bails in the long if()
> it bails out?

Will do, but going back to prefs-merge now. Do you think this bug here is subject to UI freeze restrictions?
Comment 20 Christian Persch 2018-01-30 22:02:42 UTC
(In reply to Egmont Koblinger from comment #19)
> (In reply to Christian Persch from comment #18)
> 
> > Well, the change outlined above together with the gtk fix (if it works) is
> > rather simpler than rebuilding the whole menu.
> 
> I'm afraid I missed out on a few details here (around
> GtkApplication::menubar). Is altering the menu significantly simpler for
> Unity (e.g. need to change only the model) than GNOME Shell (need to change
> the entire GTK+ widgets representing the menu)?

I don't know_ Assuming unity consumes the GtkApplication::menubar exported via dbus, then yes: simply load the unity menubar .ui file and set it as GtkApplication::menubar, and unity should do the rest.

Gnome-shell doesn't use the ::menubar at all, since it has no global menubar. When running under gnome-shell, we use our in-window menubar (plain GtkMenuBar). To change it, we need to completely clear it (destroy all menu item widgets; or just create a new menubar and replace the existing one) and re-bind to the model (and add the dynamic items back to the model).
 
> > This is different. The automatic removal applies to the *source* string, ie
> > the *un*translated string. So we know it only has one underscore.
> 
> Indeed.
> 
> > Of course this means we need to add the extra .ui file to POTFILES.in, and
> > that means it must be in the repo, not generated at build time...
> 
> I don't fancy the concept of autogenerated _and_ checked-in files, IMO it
> should be either-or. We used to have it... where exactly, maybe at box
> drawing, or vteseq? can't recall, and it caused some troubles.

Alternatively, just add a tiny file with translated strings
N_("File")
N_("Edit")
...
to the repo and POTFILES.in. I used that approach elsewhere too, and it's worked ok.
 
> > If you have a bit more time, could you what bits are set in
> > event->key.state, and on which of the conditionals it bails in the long if()
> > it bails out?
> 
> Will do, but going back to prefs-merge now. Do you think this bug here is
> subject to UI freeze restrictions?

No, it's simply a bug, which we can fix in the freeze (except hard code freeze) period.
Comment 21 Egmont Koblinger 2018-01-31 21:50:46 UTC
Created attachment 367737 [details] [review]
unity poc, doesn't work

Here's roughly how I would imagine it...

This patch intends to fix Unity only. The menu is initially installed correctly, but then isn't updated when toggling the pref.

If I replace the initial installation of the menubar with a g_idle() or g_timeout(100, ...) then it still works. Timeout of 200: sometimes works. 500: the menu doesn't appear.

Does someone really have a deadline for installing/altering the menu?? Relative to what starting point? Geez... Who's that and why?? Unity perhaps?? (Could be related to the allegedly long ago fixed https://bugs.launchpad.net/ubuntu/+source/bamf/+bug/1532226 ?)

Maybe we'll kinda have to give up, maybe make this config option a "restart required" one for unity?

Ouch, it's getting truly ugly, to the extent that I'm just inclined to give up and tell users to stop using Unity :D

Yet another terribly hacky idea which _should_ really work since it has a working PoC in our code: we toggle the "Tabs" menu based on whether there are any tabs, and it's working. So we could duplicate all the menu entries and enable half of them... but no, seriously, let's forget it :)

By the way, Alt+F does _not_ open the File menu in Unity, it passes this to the terminal. The mnemonic works for the rest of the entries. Same in Gedit, so it must be a Unity bug. At least users will be able to use Alt+F (forward-word) in bash.
Comment 22 Egmont Koblinger 2018-02-01 00:14:08 UTC
> If I replace the initial installation of the menubar with a g_idle() or
> g_timeout(100, ...) then it still works. Timeout of 200: sometimes works.
> 500: the menu doesn't appear.

Just occurred to me that my test script has a "sleep 0.2" between starting up the server and the client, and this indeed corresponds to the timer value above.

Apparently the menubar can only be modified before opening the first window. Does this make any sense?
Comment 23 Egmont Koblinger 2018-02-01 11:31:30 UTC
> Yet another terribly hacky idea which _should_ really work since it has a
> working PoC in our code: we toggle the "Tabs" menu based on whether there
> are any tabs, and it's working. So we could duplicate all the menu entries
> and enable half of them... but no, seriously, let's forget it :)

Actually, this feature also relies on an undocumented behavior. https://developer.gnome.org/gtk3/stable/GtkApplicationWindow.html mentions the "hidden-when" attribute for menu items, but not for submenus.
Comment 24 Egmont Koblinger 2018-02-01 22:04:43 UTC
Is there any other desktop environment / setup / whatever where I could test the out-of-window menubar, other than Unity (or macos)?
Comment 25 Egmont Koblinger 2018-02-01 22:30:46 UTC
There's one more conceptual problem. Hooray. (No.)

Unity, mnemonics enabled. Open prefs or profile prefs window, try to activate something using its mnemonic, e.g. "Theme _variant" using Alt+V.

Instead, the global View menu opens with all its entries being disabled.

It's silly in the first place that the menu is there at all. Is there something we could reasonably do? E.g. the Tabs menu is hidden, maybe hide all of them using the same mechanism? Anything cleaner than this?
Comment 26 Christian Persch 2018-02-01 22:42:55 UTC
Unfortunately, the gtkapplication menubar is global, ie it's the same for *all* windows. IIRRC I had proposed at least allowing to override it per window, but that wasn't taken up... (and even then unity would also need to support that)... 

The items are all insensitive because the active window (prefs dialogue) doesn't have the corresponding actions. 

The only idea I have is to try adding actions to the menubar UI for the toplevel actions, and add those in TerminalWindow (always enabled) and in the prefs window ( permanently in disabled state). That *might* work, depending on how the unity menubar works...
Comment 27 Egmont Koblinger 2018-02-01 23:06:04 UTC
> Unfortunately, the gtkapplication menubar is global, ie it's the same for
> *all* windows.

What other GNOME apps are out there using GMenu? What do they do here? E.g. do they also have their entire menu visible even when let's say the About dialog is shown?

Couldn't we create a separate GtkApplication for the prefs window? Or would that have tons of side effects, e.g. Alt+Tab not grouping these windows together?

> The items are all insensitive because the active window (prefs dialogue)
> doesn't have the corresponding actions.

(I guess some of them, at least New Window and Copy/Paste should probably work from there, but that's a different story.)

> The only idea I have is to try adding actions to the menubar UI for the
> toplevel actions, and add those in TerminalWindow (always enabled) and in
> the prefs window ( permanently in disabled state). That *might* work,
> depending on how the unity menubar works...

You mean the way we toggle the Tabs menu? That works, so we could do that, but as per comment 23 uses an undocumented behavior.

And if we're starting in this direction, we're not that far from my "terrible idea" from comment 21: make the menubar go like "File _File Edit _Edit ..." and dynamically disable every second one.


I'm also thinking of a much bigger and simpler hammer. Just use the non-mnemonic version of the menu for Ubuntu always, unconditionally.

Next to the "Enable mnemonics" checkbox we have another one: "Enable the menu accelerator key (F10 by default)". Needless to say, this doesn't quite work as this in Unity.

The checkbox toggles Shift+F10 behavior (right-click menu) just as on gnome-shell. Plain F10 is never influenced and never invokes the menu. Alt+F10 always invokes the menu regardless of this setting, it's probably a global hotkey of Unity (and then using the left/right arrows you can walk to icons at the top right, like wi-fi, battery, date etc.).

Just as we hide the "Show menubar" config from the UI, we could also hide the mnemonic one for Unity users. Let them use Alt+F10, that's it.

This set of bugs here is so complicated and it's such a mess, plus Unity is no longer Ubuntu's default, that I'd not aim for the perfect solution, I'd aim for a usable one. For me the only deal breaker is: users should be able to use Alt+letter in the terminals. Keyboard accessibility of the menu is granted by Alt+F10 anyway. If they can't choose Alt+V to open the View menu, I don't care (plus, as mentioned, due to probably a Unity bug, Alt+F doesn't open File). Anything else (like mnemonics in prefs) is nice to have but not necessary in my eyes for a deprecated desktop (but they'll get it with this suggested approach).
Comment 28 Egmont Koblinger 2018-02-01 23:10:08 UTC
> I'm also thinking of a much bigger and simpler hammer. Just use the
> non-mnemonic version of the menu for Ubuntu always, unconditionally.

s/Ubuntu/Unity/
Comment 29 Christian Persch 2018-02-01 23:29:51 UTC
(In reply to Egmont Koblinger from comment #27)
> > Unfortunately, the gtkapplication menubar is global, ie it's the same for
> > *all* windows.
> 
> What other GNOME apps are out there using GMenu? What do they do here? E.g.
> do they also have their entire menu visible even when let's say the About
> dialog is shown?

Don't know. I think no other productivity applications use GMenu yet; certainly GIMP etc don't yet.

> Couldn't we create a separate GtkApplication for the prefs window? Or would
> that have tons of side effects, e.g. Alt+Tab not grouping these windows
> together?

Could make the prefs dialogue a standalone application (separate binary) launched by g-t-s when Preferences is chosen. However then you can't test drive changes by using the memory settings backend anymore...
 
> > The items are all insensitive because the active window (prefs dialogue)
> > doesn't have the corresponding actions.
> 
> (I guess some of them, at least New Window and Copy/Paste should probably
> work from there, but that's a different story.)
> 
> > The only idea I have is to try adding actions to the menubar UI for the
> > toplevel actions, and add those in TerminalWindow (always enabled) and in
> > the prefs window ( permanently in disabled state). That *might* work,
> > depending on how the unity menubar works...
> 
> You mean the way we toggle the Tabs menu? That works, so we could do that,
> but as per comment 23 uses an undocumented behavior.
> 
> And if we're starting in this direction, we're not that far from my
> "terrible idea" from comment 21: make the menubar go like "File _File Edit
> _Edit ..." and dynamically disable every second one.

A bit saner than that, still :-)

> I'm also thinking of a much bigger and simpler hammer. Just use the
> non-mnemonic version of the menu for Ubuntu always, unconditionally.

Ok with me.
Comment 30 Egmont Koblinger 2018-02-01 23:34:21 UTC
> I think no other productivity applications use GMenu yet

I was hesitating whether to ask this... but okay... let's do... I hate to bring it up and I guess you'll hate it too... shouldn't we revert the GMenu change?

Okay, I expect the answer will be "no".

> > I'm also thinking of a much bigger and simpler hammer. Just use the
> > non-mnemonic version of the menu for Ubuntu always, unconditionally.
> 
> Ok with me.

Let's do this, then.

Plus your mnemonic modifier thingy for GNOME Shell.
Comment 31 Christian Persch 2018-02-01 23:46:20 UTC
(In reply to Egmont Koblinger from comment #30)
> > I think no other productivity applications use GMenu yet
> 
> I was hesitating whether to ask this... but okay... let's do... I hate to
> bring it up and I guess you'll hate it too... shouldn't we revert the GMenu
> change?
> 
> Okay, I expect the answer will be "no".

Right, NO :-)

Not the least since GtkUIManager was killed from gtk4, so this is required for eventually supporting gtk4.

> > > I'm also thinking of a much bigger and simpler hammer. Just use the
> > > non-mnemonic version of the menu for Ubuntu always, unconditionally.
> > 
> > Ok with me.
> 
> Let's do this, then.
> 
> Plus your mnemonic modifier thingy for GNOME Shell.

Yes, and hopefully fixing the hardcoded Alt in gtkmain.c
Comment 32 Christian Persch 2018-02-01 23:48:21 UTC
(In reply to Christian Persch from comment #31)
> (In reply to Egmont Koblinger from comment #30)
> > > I think no other productivity applications use GMenu yet
> > 
> > I was hesitating whether to ask this... but okay... let's do... I hate to
> > bring it up and I guess you'll hate it too... shouldn't we revert the GMenu
> > change?
> > 
> > Okay, I expect the answer will be "no".
> 
> Right, NO :-)
> 
> Not the least since GtkUIManager was killed from gtk4, so this is required
> for eventually supporting gtk4.

But if you think it's too buggy currently, we could branch the 3-28 version from 3-26 and only cherry-pick the pref dialogue changes to it.
Comment 33 Egmont Koblinger 2018-02-01 23:55:49 UTC
(In reply to Christian Persch from comment #32)

> But if you think it's too buggy currently, we could branch the 3-28 version
> from 3-26 and only cherry-pick the pref dialogue changes to it.

I'd perhaps vote for this approach if we saw that the issues are likely to get addressed pretty soon, which is not the case. I don't think it's going to be any better for 3.30 or 3.32.

So let's just move forward with these little hacks that will live on for quite a few cycles, and if G_UNLIKELY(it's not good enough for Ubuntu) they might choose to stick with 3.26.
Comment 34 Egmont Koblinger 2018-02-02 21:07:10 UTC
Created attachment 367841 [details] [review]
Both approaches in one, v1

This is basically your patch and mine patch combined, with these changes:

- You removed the g_settings_bind() of the shortcut aka accel key feature, I assume it was accidental, I no longer do that.

- Removed the bits from my patch that try to dynamically reinstall the menubar.

- Hide the mnemonic checkbox from the UI on Unity

Seems to work reasonably well in both desktops.

Even though the UI of the checkbox is hidden, the actual behavior is still taken from the underlying preference rather than being forced to one particular value. It _should_ not make a difference, since we're talking about the mnemonic modifier keys when no mnemonic is installed, but you can never be sure. Seems to me that "Show menubar by default in new terminals" also has this bit error-prone design. Or maybe I'm missing something.

Still in PoC state, with debug messages and such.
Comment 35 Christian Persch 2018-02-02 23:24:41 UTC
Comment on attachment 367841 [details] [review]
Both approaches in one, v1

Would using XSLT be nicer? Yes. Do I want to invest the time to come up with the XSLT stylesheet? No way!  So let's go with this simple sed job :-)

+  g_printerr("mnemonics now %sabled\n", enabled ? "en" : "dis");

Remove the debug spew.

+  if (enabled)
+    gtk_window_set_mnemonic_modifier (GTK_WINDOW (window), GDK_MOD1_MASK);
+  else
+    gtk_window_set_mnemonic_modifier (GTK_WINDOW (window), GDK_MODIFIER_MASK);

I think I figured out the problem with the gtk patch not working: GDK_MODIFIER_MASK includes GDK_RELEASE_MASK. So replacing this with gtk_window_set_mnemonic_modifier (GTK_WINDOW (window), GDK_MODIFIER_MASK & ~GDK_RELEASE_MASK); should make the gtk patch work too.
Comment 36 Egmont Koblinger 2018-02-03 16:32:27 UTC
(In reply to Christian Persch from comment #35)

> I think I figured out the problem with the gtk patch not working:
> GDK_MODIFIER_MASK includes GDK_RELEASE_MASK. So replacing this with
> gtk_window_set_mnemonic_modifier (GTK_WINDOW (window), GDK_MODIFIER_MASK &
> ~GDK_RELEASE_MASK); should make the gtk patch work too.

The GTK+ patch seems to be more problematic, e.g. Alt on its own doesn't bring up the underscores anymore (without any g-t patch from this bug), but adding Shift to it (in this order only) brings it up. I guess key.keyval of GDK_KEY_Alt_X is not that interchangeable with key.state of GDK_MOD1_MASK. Needs further investigation.

That being said, sure we should remove RELEASE_MASK from our mask.
Comment 37 Egmont Koblinger 2018-02-03 16:42:57 UTC
Created attachment 367860 [details] [review]
Both approaches in one, v2

Unset RELEASE_MASK as discussed. Removed debug stuff.

Removed an assertion from your code which I felt you didn't mean to submit. Let me know if I should bring it back (to the top of that method then, I guess).
Comment 38 Egmont Koblinger 2018-02-03 16:50:39 UTC
Misleading typo:

The GTK+ patch seems to be more problematic, e.g. Alt on its own doesn't bring up the underscores anymore (without any g-t patch from this bug), but **pressing** Shift to it (in this order only) brings it up. [...]
Comment 39 Christian Persch 2018-02-03 17:37:10 UTC
Turns out that event->key.state excludes the modifier that's been pressed and which is present in key.keyval  :-(
Comment 40 Christian Persch 2018-02-03 18:04:52 UTC
Created attachment 367861 [details] [review]
next try of a gtk patch (conceptual, not even compile tested)

With that, we should even be able to simply use 0 instead of GDK_MODIFIER_MASK to disable.
Comment 41 Egmont Koblinger 2018-02-03 18:07:19 UTC
> next try of a gtk patch (conceptual, not even compile tested)

I'll test it a bit later

> With that, we should even be able to simply use 0

... whenever we require a fixed gtk, which is many years from now. :)
Comment 42 Egmont Koblinger 2018-02-04 00:28:48 UTC
Created attachment 367869 [details] [review]
Both approaches in one, v3

Updated against current master (with prefs merge). Added generated files to Makefile.am for the sake of make clean, .gitignore etc.

Going to add comments to the source before commenting. 1:30am is not the right time for that :)
Comment 43 Egmont Koblinger 2018-02-04 00:29:41 UTC
... before *committing*. It's sure not the right time :)
Comment 44 Egmont Koblinger 2018-02-04 19:58:17 UTC
Created attachment 367888 [details] [review]
Both approaches in one, v4

Comments added. (And changed sed's separator to a nicer one :-).)

Submitting...
Comment 45 Khurshid Alam 2018-08-07 16:38:30 UTC
@Egmont Koblinger

Why no mnemonics on unity? This prevents user to use ALT+F, ALT+E which are standard global shortcuts.

Problem it's not just unity. If you use gnome-flashback with global-menu (indicator-appmenu enabled with unity-gtk3-module installed), it won't work even though in flashback you have the "enable-mnemonics" settings under preference.

Note both gnome-flashback and Unity are still maintained while Unity became community project. 

In flashback compiz session we still have the settings in preference while in Unity we don't? 

This creates confusion among users. Will you consider enabling mnemonics for Unity as well?

https://bugs.launchpad.net/ubuntu/+source/unity/+bug/1734817
Comment 46 Egmont Koblinger 2018-08-07 20:28:52 UTC
In bug 745329 gnome-terminal switched to a newer architecture for its menus, GMenu. It has its advantages, and unfortunately its downsides too.

With this new architecture, I ran into at least three problems on Unity:
 - As mentioned in the last paragraph of comment 21: Alt+F didn't work, although the other menu shortcuts did.
 - As mentioned in comment 25: The shortcuts activate the menu even when they shouldn't.
 - Mentioned throughout various comments: Changing the config option would have needed application restart.

These altogether are quite an issue to justify going for the simple solution (i.e. drop the mnemonics), rather than a series of ugly, still broken, hard to maintain hacks. Let's also mention that:
 - Unity is no longer Ubuntu's default desktop, so not high priority for us (probably the reason we cared at all is because I still use it);
 - enabling Alt+F etc. conflicts with their use inside the terminals, which most people want;
 - you can still easily focus the menu using Alt+F10.

Let me know when Unity (or whichever component is responsible) addresses the aforementioned three issues that blocked us from implementing this feature correctly with GMenu; I'd be happy to return!

gnome-flashback was totally out of my radar, and will remain so, I'm sorry but I have no capacity to test gnome-terminal on yet another desktop.
Comment 47 John Pye 2018-08-07 23:32:11 UTC
I found it to be weird and disconcerting when I discovered that mnemonics were disabled *by default* in gnome-terminal. 

Alt+key mnemonics have been the reliable fallback for keyboard navigation in basically all Windows/Linux environments going back to I don't know when, but probably Windows 3. They're really useful!

Console/curses apps, if anything, have *copied* this obvious approach to keyboard user-friendliness. But now,  presumably because some users got used to keyboard navigation in those console apps, they have managed to convince GUI developers that it is OK to ruin the consistent keyboard navigation technique in the far more prevalent GUI apps!

For the console app users there are other sensible approaches to keyboard navigation that should not override the GUI design principle of Alt+key menus. For example, the console-based ELinks browser uses ESC to activate menus, as well as F10. Also, when gnome-terminal is full-screen, it allows ELink to grab the Alt+key options as well. Seems like it's covered! 

What other console apps are so demanding that they can drive the design of gnome-terminal to give up on the menu mnemonics? 

My main reason for sticking with Unity is the menubars, honestly! GNOME 3 is removing them from Evince, Gedit, etc... so please please could we at least ensure that Unity preserves the menubars somehow?

If the GNOME developers don't have strong feelings about Unity users, and if having switchable behaviour is too hard, then please consider the option of KEEPING the mnemonics on Unity, rather than disabling them by default.
Comment 48 Egmont Koblinger 2018-08-08 08:54:54 UTC
I'm really unsure the chronological order is what you describe. Terminals have been around for about 40 years perhaps, GNOME for 20-ish. Not sure where the Meta/Alt key and its behavior as an ESC prefix appeared, but I'm almost sure it predates GNOME.

All emulators, not just terminal emulators (e.g. vmware, vnc...) suffer from the same conceptual problem, there's a conflict whether the shortcut should be handled by the emulator itself, or sent to whatever is running inside.

For the same reason, gnome-terminal has nonstandard shortcuts for other actions too, just as Ctrl+Shift+C/V for copy-paste. After using gnome-terminal for about 15 years now it still drives me crazy, I just don't know what we could do to fix it.

The generic take, as I understand from user feedbacks, is that sending Alt+F, F10 etc. to the app inside is a much more typical demand. E.g. one might want to use Alt+F for jumping forward by a word pretty regularly, even multiple times per minute inside their favorite shell, text editor etc. (even holding it down longer, which can't be done with the ESC F approach). I can't imagine a reason for invoking the menu anywhere near that frequently.

Please understand that I also find it very important to be able to access the menu by keys. It's just a different key, namely Alt+F10. For most people it's much less inconvenient than having to find workaround keys inside each and every (and not just one arbitrarily picked) application within the terminal.

That being said, I'd be more than happy to reintroduce the config option if we could make it work without problems (and without reverting the GMenu architectural change). It needs Unity and perhaps also GTK+ work, not sure about the latter. I have neither capacity nor motivation to help fix the underlying infrastructure, sorry for that, but I'd be happy to cooperate with a Unity developer.
Comment 49 Khurshid Alam 2018-08-08 16:57:10 UTC
@Egmont Koblinger

Thanks for your quick response. I am happy to help. 

What I can observe, in flashback compiz session, If I use indicator-appmenu, 

It is exporting gnome-shell vertical menu instead of traditional horizontal menubar. See https://i.imgur.com/HoSxLiu.png

If I force it using 

gsettings set org.gnome.settings-daemon.plugins.xsettings overrides '@a{sv} {"Gtk/ShellShowsMenubar": <int32 1>}

It displays both vertical & horizontal menu and Alt+F stops working. May be same thing is happening in unity except for unity, "ShellShowsAppmenu" is not set. I will ask flashback maintainer if it is a problem with flashback-session.

For the time being, this is what I am going to do:

1) Revert this patch and check if it same with unity.

2) Try to find out why Alt+F doesn't work with gmenu for terminal. Other apps also use gmenu and it works for them. 

3) This bug is closed. So I will open a new bug and continue the discussion there.

Thanks.
Comment 50 Egmont Koblinger 2018-08-09 09:52:32 UTC
Khurshid, thanks in advance for your work on this!

> Other apps also use gmenu and it works for them. 

As per the linked Ubuntu bug, Alt+F in gedit works for you on Cosmic, but not for me on Bionic. Could easily be a difference in our settings of course, but perhaps Ubuntu has already fixed it for Cosmic?
Comment 51 Khurshid Alam 2018-11-06 14:25:44 UTC
Update:

1) Alt+F most times doesn't work (for any app): 

This was actually the issue with the unity settings daemon. Unity uses old legacy key grabber code depending on XDG_SESSION which was changed from "Ubuntu" to "Unity" in 17.10. So u-s-d was failing to grab keys when grab is already pending.

I fixed that (https://bazaar.launchpad.net/~khurshid-alam/unity-settings-daemon/media-keys-fix/revision/4186).

Once this lands on 19.04, Alt+F will always work. I will open a bug on gitlab once this happens.


2) mnemonics are enabled in search/preference.

I partially reverted the commit (https://git.launchpad.net/~khurshid-alam/ubuntu/+source/gnome-terminal/commit/?id=a6d7680bdffe498840bc1ce0e3c00521bfca058d)

Now with this, no Alt+F in search menu. But it is still active in pref, but so is for gedit.


3) Alt+F can't be easily disabled.

This could be a bug indicator-appmenu or unity-gtk-module. Though it works in flashback-session. I haven't found any other app which has option to disable mnemonics from gui. From the example I have seen, If a app wants to disable mnemonics they simply convert "_File" to "File" in ui files.



Based on above, I suggest to re-enable mnemonics in Unity, but keep it removed from preference. We will use a gsettings-overide to keep it enabled for unity, (glib in ubuntu supports per session based overrides since 17.10). 

Unity users are not really concerned about disabling Alt+F. They expect it to work everywhere for all apps. That's how it is from the beginning.


Also, gtk-shell-shows-menubar may not be good idea as other non-unity desktops can also use global menu. If this is intended only for unity, then may be using  XDG_CURRENT_DESKTOP is better.
Comment 52 Egmont Koblinger 2018-11-06 14:57:35 UTC
> 1) Alt+F most times doesn't work (for any app):

Based on your description and the patch, I have no idea how this bug only affected Alt+F and no other Alt+letter combinations (or perhaps only the first menu item). Anyway, I'm glad you've fixed it, thanks!

> Unity users are not really concerned about disabling Alt+F. They expect it to
> work everywhere for all apps. That's how it is from the beginning.

I wouldn't generalize like this. Terminal emulators are special, have always been due to their nature, similarly to other kinds of emulators (like VMware). There's a conflict whether Alt+F should take effect inside or outside the terminal, such conflict doesn't exist in most of the apps. As I take from user reports, I have the strong feeling that "inside" is the more frequent request (to make it work in bash and friends) when it comes to a terminal emulator, and I honestly doubt that the choice of window manager can make a substantial difference here.

> If a app wants to disable mnemonics they simply convert "_File" to "File" in ui files.

That's what gnome-terminal does now for Unity. Alas it cannot toggle runtime (and we might not have even implemented respecting the global setting at startup, I can't recall). It would be great if you could figure out and fix why the menu can't be toggled between "File" and "_File" runtime.

> Also, gtk-shell-shows-menubar may not be good idea as other non-unity desktops can also use global menu.

We cannot test all the desktops out there. Our thinking was: if anyone else (we don't care who) shows the global menu, we don't have to.

> Once this lands on 19.04, Alt+F will always work. I will open a bug on gitlab

Thanks for your work and please keep us posted of further findings!

Please note that I typically upgrade to Ubuntu about a month before its release, which is pretty much the beginning of freeze period for GNOME. That is, whatever is changed/fixed in Unity 7, gnome-terminal will most likely only adjust one cycle later.