hid/gtk: Replace hard-coded keynames translation-table

[Felix Ruoff]

I have thought about the keyname translation for the gtk gui. Today this table replaces special accelerator-keys of the gpcb-menu.res file to their gtk-name. (The affected keys are '=', '.', ':', '|', '/'. '[' and ']'.)

In my opinion it is not much more complicated for the user to use the gtk keynames in the gpcb-menu.res file. If he/she uses them, there will be no confusion if some keys are missing in the translation-table (e.g. '+' and '-' are missing, as noted in LP:699493) and others are supported (as the given above).

The appended patch removes the code for the translation-table and replaces the used keys in the gpcb-menu.res by the gtk keynames. This patch also adds a hint to the usage of accelerators and the location of the gdk-keynames file to the gpcb-menu.res file.

[Peter Clifton (pcjc2)]

I'm not in favour of (read - won't apply unless strongly persuaded otherwise) any patch which causes a divergence in accepted syntax / semantics of the pcb-menu.res and gpcb-menu.res files, which this patch seems to create.

[Peter Clifton (pcjc2)]

I wonder if we can make GDK do the leg work of conversion... (UNTESTED)

Could you investigate whether the following would help us?

key_char = "|"; // For example

gunichar unichar = g_utf8_get_char (key_char);
// For best robustness, use g_utf8_get_char_validated() above and handle in-band error returns (gunichar)-1 and (gunichar)-2
unsigned int keyval = gdk_unicode_to_keyval (unicode_point);
const char *keyval_name = gdk_keyval_name (keyval);

[Felix Ruoff (felixruoff)]

Thanks for your replies, Peter! I will look into that as soon as possible.
Is it good style to set 'Assigned to' to me for this bug or should this option just be set to/by the commiters?

[Peter Clifton (pcjc2)]

I didn't want to assign you unless you were willing.. but clearly you are, so assigning now.

Feel free to assign yourself to bugs you intend to fix, or work on fixes for.

[Peter Clifton (pcjc2)]

I have no idea how LP managed to unassign you in the same action as me assigning you! (See comment #5)

[Felix Ruoff (felixruoff)]

I have attached a new patch which replaces the keyname table with g/gdk - functions as Peter Clifton suggested.

With this patch applied, keynames (like 'period', 'bracketleft' or 'BackSpace') and thir corresponding characters (e.g. '.', '[', 'e', ...) can both be used in the gpcb-menu.res file. All keynames and their corresponding characters of the <gdk/gdkkeysyms.h> - headerfile can be used.

Please feel free to give any more comments, wishes and hints to improve this patch!

If this (or a similar) patch will be applied, this also closes LP:699493

[Peter Clifton (pcjc2)]

The maximum string length accounting of this portion of the patch looks wrong.

+ const char *keyval_name = gdk_keyval_name(keyval);
+ strncat (accel, keyval_name, strlen(keyval_name));
+ accel_n -= strlen (keyval_name);

from the strncat manpage:
       If src contains n or more characters, strncat() writes n+1 characters to dest (n from src plus the termi‐
       nating null byte). Therefore, the size of dest must be at least strlen(dest)+n+1.

"accel_n" is meant to count how much room is left in the string, and should be passed to strncat as the third argument to ensure it doesn't walk off the end. This (existing) code is not very nice, to put it mildly.

I think you need to:
- strncat (accel, keyval_name, strlen(keyval_name));
+ strncat (accel, keyval_name, accel_n);

On the plus side, it is in the GTK HID, so we have access to some much more friendly string manipulation functions.
If you fancied doing so, you could (not suggesting should.. we can apply the current patch once fixed up), consider re-writing this function using g_ string handlign functions such as g_strconcat () ?

See: http://library.gnome.org/devel/glib/unstable/glib-String-Utility-Functions.html

(Be sure to remember you need to g_free() the resulting memory, not free()

[Felix Ruoff (felixruoff)]

Thanks for spotting that out, Peter!
I have reworked this patch a bit with using these g_ string handling functions to get rid of the 'remaining character counter' accel_n. Hope, I have not introduced any new errors...

Any feedback, as usual, is very welcome!

[Peter Clifton (pcjc2)]

I don't like the sound of this..

So, the keyval can be given in two ways - as character or as keyname.

":" should work "colon" should not work. We must avoid introducing diversion between accepted syntax in gpcbmenu.res and pcbmenu.res

Only recognised modifier keys should be identified by name. If we want to start introducing keys by name type semantics.. we need to rework the core / lesstif handlers as well.

[Felix Ruoff (felixruoff)]

I have had a look into the lesstif code today to find out, if I can implement using keynames for the pcbmenu.res/lesstif, too. There I discovered, that this is already implemented there! I tried it with some of the used 'special-keys' and it works out of the box.

For this reason I think, no more rework of the lesstif - code is needed. What do you, Peter, mean with the core-handlers which should be reworked? Are there other places that needs reworking?

[Peter Clifton (pcjc2)]

What special names did you try in Lesstif?

[Felix Ruoff (felixruoff)]

I tried it with plus, period, colon and minus for the Lesstif-GUI. All this works.

But further testing today shows problems with some other keynames (e.g. bracketright). As far as I can see, some of the problems are related to my german keyboard layout. I need some time to think about it and to evaluate the problems before I will start to implement something for this. (bracketright/left needs to use the 3. level chosser button)

Here are my actual thoughts (I will send them to the geda-user mailing list, too, to get more input, but I think, here also is the right place for this to have all related things for this bug at one place):

Today, key-modifiers (shift, control, mod1, ...) and keys (like 'a', 'b', ...) are handled seperately. This will give problems with some of the default-accelerators if used on different keyboard-layouts.
One example: On an american keyboard (querty) you have the a button for '='. So, the accelerator 'shift<Key>=' is no problem. With a german keyboard (qwertz) you will get the equal sign by hitting shift+0 (zero). With this keyboard layout it is impossible to use the accelerator 'shift<Key>=' which is applied to "Auto-Optimize" in the default pcb-menu.res. This problem exists for the lesstif and the gtk gui (see also LP:70748 which is related to this problem).

This lets me think, that just using keynames for accelerators are a better choice for the future. The needed strings for the (g)pcb-menu.res file are the same for the gtk and lesstif gui, because both take their keyname table from the X11 environement (see http://tronche.com/gui/x/xlib/input/keyboard-encoding.html#KeySym for Lesstif and http://library.gnome.org/devel/gdk/stable/gdk-Keyboard-Handling.html for the GTK gui). With this, it might be possible to distinguish between small and large/capital? letters and to be (nearly) sure to have all used accelerators on everyones keyboard. Then some (all) of the modifiers would be invalid for accelerators (I think, perhaps they will still be needed for mouse-actions and perhaps some other stuff).

This are just my thoughts of today. Comments are very welcome.

[DJ Delorie (djdelorie)]

I suspect that, for non-US users, you'd want a whole new [g]pcb-menu.res *anyway* that has menu entries in the appropriate language. Such a replacement file could have the right accelerators to match the keyboards.

Perhaps we should be discussing a way to internationalize the menus, not a way to specify key sequences agnostically.

[Felix Ruoff (felixruoff)]

I think, adding new [g]pcb-menu.res files for non-US users will give problems with support and documentation.
Then you have to distinguish between the languages like:
"If you are using an US-keyboard, this the accelerator for function <function-name> is key <A>, with an german keyboard its key <B> and for other languages have a look at your [g]pcb-menu.res file." I would not like that...

Or did I missunderstand you?

[DJ Delorie (djdelorie)]

I meant, hide the keyboard-specific ways of making "shift-3" work in different resource files, so that the documentation is the same for all languages and it "just works".

As for mnemonics, they'll have to be different for each language anyway, but we don't document those.

[Felix Ruoff (felixruoff)]

I am sorry to tell, that I don't see a really good solution for this problem and (this is the main point) I don't have the time to work at this item in the next weeks. For this reason, I deassigne me for this bug and hope, someone else like to work with it.