(In reply to Eduard Braun from comment #0)
> I'm just in the process of tracking down a bug that breaks shortcut
> functionality in Inkscape when built with gtk3 and noticed that
> gtk_accelerator_get_label() seems broken.

How so? What are you trying to do with the returned label? Ultimately it comes from GtkAccelLabel and it only meant to

> [convert] an accelerator keyval and modifier mask into a string which can be used to represent the accelerator to the user.

not necessarily one that can be consumed by any other API in any reliable way.


That said, it's not immediately clear why you don't get the result you want anyway. The responsible function here is ultimately _gtk_accel_label_class_get_accelerator_label, so maybe there are clues to be found in there.

Guys, what's with the attitude? Why is every bug in the GTK+ bugtracker answered with something like "Why? What are you trying to do? You're likely doing it wrong! This almost certainly is not a GTK issue"?

This bug is as simple as it gets (and as described in the initial comment):
The returned "string which can be used to represent the accelerator to the user" shows character names like "section/degree/EuroSign" (which are not exactly representable, are they?) instead of the proper character "§/!/€".

Earlier versions of GTK+ worked properly in this case.
Recent versions don't.

Now argue whatever you want, it's a plain and simple regression.

(In reply to Eduard Braun from comment #2)
> Guys, what's with the attitude? Why is every bug in the GTK+ bugtracker
> answered with something like "Why? What are you trying to do? You're likely
> doing it wrong! This almost certainly is not a GTK issue"?

I apologise if it came across as attitude. That was not the intention; I simply genuinely was interested to know what this was being used for.

You will note that I only asked what you were doing. I didn't say you were likely doing it wrong, nor that it wasn't necessarily a GTK+ issue.

I don't know what experiences you've had in the past, but it'd be nice if they weren't transferred onto me for just asking a simple question.


> This bug is as simple as it gets (and as described in the initial comment):
> The returned "string which can be used to represent the accelerator to the
> user" shows character names like "section/degree/EuroSign" (which are not
> exactly representable, are they?) instead of the proper character "§/!/€".
> 
> Earlier versions of GTK+ worked properly in this case.
> Recent versions don't.
> 
> Now argue whatever you want, it's a plain and simple regression.

It wasn't immediately clear from your wording that this had worked in the past. For instance, you might have been using it to replace a different method of doing this from GTK+ 2. Thanks for the clarification.


In OSS generally, if users/contributors/maintainers could all stop being so reflexively distrustful of and aggressive with other groups, that would really help us all out, right?

The problem to me seems to be that, for symbols like the ones you mentioned, and also £ and etc, we take the 2nd branch here because the guint value of the gunichar is >= 128: 

gtk/gtkaccellabel.c:818:

  ch = gdk_keyval_to_unicode (accelerator_key);
  if (ch && ch < 0x80 && (g_unichar_isgraph (ch) || ch == ' '))
    {
      if (seen_mod)
        g_string_append (gstring, klass->mod_separator);

      switch (ch)
	{
	case ' ':
	  g_string_append (gstring, C_("keyboard label", "Space"));
	  break;
	case '\\':
	  g_string_append (gstring, C_("keyboard label", "Backslash"));
	  break;
	default:
	  g_string_append_unichar (gstring, g_unichar_toupper (ch));
	  break;
	}
    }
  else if (!append_keyval_symbol (accelerator_key, gstring))
    {
      gchar *tmp;

      tmp = gdk_keyval_name (gdk_keyval_to_lower (accelerator_key));


That condition was changed in this commit:

commit f760538f17673c5bd7fec792be2f1abf8148fc32
Author: Javier Jardón <jjardon@gnome.org>
Date:   Thu May 20 02:39:45 2010 +0200

    gtkaccellabel: Remove unused class members

    Fixes https://bugzilla.gnome.org/show_bug.cgi?id=97414

diff --git a/gtk/gtkaccellabel.c b/gtk/gtkaccellabel.c
index 9193ea5bf1..716e8d7cd0 100644
--- a/gtk/gtkaccellabel.c
+++ b/gtk/gtkaccellabel.c
@@ -180,9 +180,6 @@ gtk_accel_label_class_init (GtkAccelLabelClass *class)

 #endif /* GDK_WINDOWING_QUARTZ */

-  class->accel_seperator = g_strdup (" / ");
-  class->latin1_to_char = TRUE;
-
   g_object_class_install_property (gobject_class,
                                    PROP_ACCEL_CLOSURE,
                                    g_param_spec_boxed ("accel-closure",
@@ -802,8 +799,7 @@ _gtk_accel_label_class_get_accelerator_label (GtkAccelLabelClass *klass,
     g_string_append (gstring, klass->mod_separator);

   ch = gdk_keyval_to_unicode (accelerator_key);
-  if (ch && (g_unichar_isgraph (ch) || ch == ' ') &&
-      (ch < 0x80 || klass->latin1_to_char))
+  if (ch && ch < 0x80 && (g_unichar_isgraph (ch) || ch == ' '))


That change to the condition has no rationale, was unrelated to the message, and is quite possibly incorrect. Let's figure out how to replicate the old condition and see how we get on.


Of course, it's also possible that this was deliberate but just not explained; I don't know and wasn't there.

(In reply to Daniel Boles from comment #4)
>    ch = gdk_keyval_to_unicode (accelerator_key);
> -  if (ch && (g_unichar_isgraph (ch) || ch == ' ') &&
> -      (ch < 0x80 || klass->latin1_to_char))
> +  if (ch && ch < 0x80 && (g_unichar_isgraph (ch) || ch == ' '))

In GTK+ 2.24, klass->latin1_to_char was unconditionally set to TRUE
in the class init function, so effectively the ch < 0x80 never mattered before.
Therefore, it should probably be removed from the condition now.

Created attachment 356930 [details] [review]
AccelLabel: Fix displaying accel unichars >= 0x80

    AccelLabel: Fix displaying accel unichars >= 0x80

    This condition stopped accel keyvals with gunichar value >= 0x80 from
    being rendered as symbols, instead falling back to their keysym name.

    In GTK+ 2, the ch < 0x80 was ORd with klass->latin1_to_char, and that
    was unconditionally set to TRUE in the class init function, so
    effectively the ch < 0x80 never mattered before or served any purpose.

    So, when klass->latin1_to_char was deleted from the class in commit
    f760538f17673c5bd7fec792be2f1abf8148fc32, the sense of this condition
    changed. This annoys users who were used to the old behaviour and using
    it for parsing, etc.

    So, the < 0x80 should probably be removed to restore the old behaviour.


Please test this and report back.

Also, anyone who can review, please do. I don't want to comment much on the sense here; I'd prefer anyone who understands all this history behind this to explain if this is correct. However, the removal of the always-TRUE latin1_to_char certainly seems to have been hasty and considerably changed the sense of this function.

Another thing I'm concerned about is the possibility that someone now relies on the current (pretty clearly unintended and wrong) behaviour of GTK+ 3, and would be opening another ticket if we fix this one... While I lean towards that being unlikely and not necessarily a reason not to fix it, it seemed worth pondering.

tangential: This function is broken on GTK+ 4, for a different reason, because (on X11 here) it is adding Mod2 to the returned label despite that I am not pressing it

sorry, nvm - it must be the test program not filtering to default_mod_mask, and GTK+ 3 is the same anyway

to confirm: the patch, of course, appears to restore the previous behaviour, as desired by Eduard.

Having thought about it more, I can't imagine any way in which this would've been intentional or any way that it's not a huge step backwards in usability, and so:

(In reply to Eduard Braun from comment #2)
> The returned "string which can be used to represent the accelerator to the
> user" shows character names like "section/degree/EuroSign" (which are not
> exactly representable, are they?) instead of the proper character "§/!/€".
> 
> [...] it's a plain and simple regression.

Especially because, as was to become clear, these are raw keyval names only (not e.g. some other kind of user-facing key name), and therefore not translatable.

I've pushed the fix to gtk-3-22 and master.


Eduard: Well spotted, or so it seems because somehow no one else noticed this since 2010 :S I guess everyone else uses very mundane accelerators. Thanks for your detailed report, test case, and explanation.

Thank you very much for looking into this Daniel. I just tested on Windows 10 with a patched build of gtk3-3.22.16 (built using MSYS2/mingw-w64).

The patch seems to work and restores the gtk2-behavior.
I didn't find any obvious regressions.

There are still some characters for which only the character name is shown, e.g. "backslash" ("\") and most of the "dead" keys like "dead circumflex" ("^") and "dead acute/grave" ("´/`") but this was already the case earlier and might have other reasons.


<offtopic>
Sorry if I snapped at you initially but your question "What are you trying to do" and the note "...[it is] only meant to..." seemed to indicate that you might in fact have suggested that "I was doing it wrong".

Maybe I am in fact a bit prejudiced by now (my experiences are not exactly positive so far - if you want to know why Dirk Hohndel boiled it down quite nicely [1]; the comment on "if you are on Windows" made me actually lough out loud as it really hits the nail right on the head). As a result I probably interpreted your comment incorrectly and again I'm sorry for that, as in fact your reactions were a badly needed positive experience.

If you're still interested in what we use "gtk_accelerator_get_label()" for you can have a look at [2]. We really only use it to create labels that are shown in different places in the UI. So - at least that part - we're actually not doing wrong (although I admit that the rest of the shortcut system does basically everything different than it might have been intended by GTK+)
</offtopic>

[1] http://mirror.linux.org.au/pub/linux.conf.au/2014/Thursday/83-Gtk_to_Qt_-_a_strange_journey_-_Dirk_Hohndel.mp4
[2] https://gitlab.com/inkscape/inkscape/blob/f4a9680414ed93edc2773df843b8122552f11b3e/src/shortcuts.cpp#L869-871

You're welcome, and thanks for the other info!

To be honest, I wasn't consciously aware that gtk_accelerator_get_label() existed, otherwise I might not have gone to the trouble of putting a GtkShortcutLabel in the button tooltips of my main project... but now that I did, it looks really nice. :) (Let's just not talk about my first attempt, which hacked out a textual representation from GtkShortcutLabel's child labels...)


(In reply to Eduard Braun from comment #12)
> There are still some characters for which only the character name is shown,
> e.g. "backslash" ("\") and most of the "dead" keys like "dead circumflex"
> ("^") and "dead acute/grave" ("´/`") but this was already the case earlier
> and might have other reasons.

The special-case for \ to Backslash goes all the way back to the initial commit, so no rationale is immediately obvious. On a quick search I can't see any bug requesting a change or clarification here, so you might want to file one for it and see what someone else thinks.

While there, you could ask about the dead characters too, although they are not special-cased in gtkaccellabel.c, so it's GLib that makes the decision that they're non-graphical - probably intentionally, but GLib would be the place to ask in any case.