Nice idea. However, currently, stock items only handle a single accelerator

Created attachment 89555 [details] [review]
gtk+-stock-mmkeys.patch

A first pass at it. To note:
- I can extend the current approach in gtkstock.c to also support translated stock icons without mnemonics, which could then be used for window titles for example.
- There's no actual use of gtk_stock_lookup_special_key() yet, I plan on adding it to GtkActionGroup and GtkImageMenuItem.
- I've added the XF86 multimedia keys to the GDK keysyms. I know this has been rejected in the past, but it's the only way to support those without creating windowing-system specific hacks. Any alternatives?

When I inquired on xorg list about the status of the XF86 keysyms, jg pretty much said that they are official, so I have no reservations about using them at this point.

Note that there's also a bunch of "Sun" keysyms in X11/Sunkeysym.h that I intend on adding support for, but this time directly in GDK's X11 backend (ie. just matching SunXK_Copy to GDK_Copy for example).

(In reply to comment #3)
> When I inquired on xorg list about the status of the XF86 keysyms, jg pretty
> much said that they are official, so I have no reservations about using them at
> this point.

Great! Does the rest of the approach seem sensible, or should I just break ABI and move those extra items to the GtkStockItem struct? (possibly adding some necessary padding to avoid problems later on)

Haven't had a chance to look at the patch yet

Hmm, there are some issues here

- The patch as is only allows special keys for builtin stock items, not
  for stock items added by the application

- It only allows a single extra keysym, and no modifiers

I guess I would prefer a patch that doesn't have these restrictions.
Maybe something like

gtk_stock_add_accelerator (const gchar     *stock_id, 
                           GdkModifierType  modifier,
                           guint            keyval);

We probably also need a way to add extra accelerators to actions,
maybe

gtk_action_add_accelerator (GtkAction       *action,
                            GdkModifierType  modifier,
                            guint            keyval);

(In reply to comment #6)
> Hmm, there are some issues here
> 
> - The patch as is only allows special keys for builtin stock items, not
>   for stock items added by the application

Having an API for external stock items was planned.

> - It only allows a single extra keysym, and no modifiers

Do we want to be able to support more than one extra keysym per action? I'll add the ability to have modifiers.

> I guess I would prefer a patch that doesn't have these restrictions.
> Maybe something like
> 
> gtk_stock_add_accelerator (const gchar     *stock_id, 
>                            GdkModifierType  modifier,
>                            guint            keyval);

OK.

> We probably also need a way to add extra accelerators to actions,
> maybe
> 
> gtk_action_add_accelerator (GtkAction       *action,
>                             GdkModifierType  modifier,
>                             guint            keyval);
> 

GtkAction seems to only support one accelerator per action, so this would need to be changed before we can support multiple keysyms for a widget in GtkAction.

Created attachment 89613 [details] [review]
gtk+-stock-mmkeys-2.patch

- Handle extra keys with modifiers
- Add gtk_stock_add_accelerator for external stock items to be able to add modifiers
- Support translating Sun keycodes to GDK ones in the X11 backend
- Add support for multimedia keys to the old menu system (GtkImageMenuItem)

To do:
- What does GDK_Clear do? How is it different from XF86_XKClear?
- Need to add a whole load of keys to keynames.txt (do we need to?)
- Add multi-keys support to GtkAction (gtk_action_set_accel_path and friends)

I'd need to know whether it's enough for a GtkAction to support only 2 shortcut keys, or whether we want more. If we want more, I'd need to change the GtkStockItem API to handle > 1 extra key combos properly.

I had in mind that you should be able to call gtk_stock_add_accelerator multiple
times to have more than one additional accelerator; same for actions.

(In reply to comment #9)
> I'd need to know whether it's enough for a GtkAction to support only 2 shortcut
> keys, or whether we want more. If we want more, I'd need to change the
> GtkStockItem API to handle > 1 extra key combos properly.

In Epiphany we have actions with more than 2 accels (e.g. Reload: the main Ctrl-R, F5 for IE compat, XF86XK_Refresh; others have even more accels).

(In reply to comment #11)
> (In reply to comment #9)
> > I'd need to know whether it's enough for a GtkAction to support only 2 shortcut
> > keys, or whether we want more. If we want more, I'd need to change the
> > GtkStockItem API to handle > 1 extra key combos properly.
> 
> In Epiphany we have actions with more than 2 accels (e.g. Reload: the main
> Ctrl-R, F5 for IE compat, XF86XK_Refresh; others have even more accels).

Makes sense. Will make it a slight bit more complicated though...

Created attachment 89761 [details] [review]
gtk+-stock-mmkeys-3.patch

Changes:
- Support multiple secondary accelerators in GtkStock
- Add test in gtk-demo's appwindow
- Make GtkStockItemKey public

To do:
- What does GDK_Clear do? How is it different from XF86_XKClear?
- Need to add a whole load of keys to keynames.txt (do we need to?)
- Add multiple accelerator support to GtkAction (gtk_action_set_accel_path and friends)
- Add multiple accelerator support to GtkMenuItem

Ok, lets pick this up again before it turns 1...

First of all, lets break this into multiple patches:

- keysym update
- sun keysym translation
- multiple stock accelerators 


Then, I think I'd prefer to keep GtkStockItemKey out of the public api.
Can we make it just 

void
gtk_stock_add_secondary_accelerator (const gchar     *stock_id,
                                     GdkModifierType  modifier,
                                     guint            keyval)

?

For the builtin items, I think I would prefer to add a second array 
instead of extending the current array, since most items don't have a secondary accelerator.

Should we use a GSList for the extra accelerators ?

I wonder about accel labels - should they show all accelerators ?

I think it is fair to say we aren't going to do this now that stock items have been deprecated.