Created attachment 202587 [details] [review]
initial patch

Left to do here:

- clean up
- check memory handling
- consider if we want to have accels inside menu xml, or separate

Note that I've added F11 for fullscreen to test this in bloatpad - Copy/Paste keybindings already work, because they are set up as key bindings by text view.
We probably want to add them anyway, to display the accels in the menu.

More left to do: pass the 'target' parameter when activating

I find the idea of putting the accel inside the menu interesting.

good:

  - automatic documentation of all shortcuts in a very authoritative way

  - no need for an extra API

bad:

  - no way to add "secret" shortcuts (maybe that's not so bad...)

  - no way to have more than one shortcut per action without multiple items

  - we need to walk through the menus locally even if they are not shown
    (ie: because the shell shows them) to setup the accels

  - even if they are shown, I intend to lazy-construct the submenus, so we
    would have to do an initial pre-pass in order to discover the accels.

  - we need to monitor for changes to all the menus to add new accels...

I particularly like the part about how the accel is only defined in one place (ie: no need to both add the accel itself and also add the proper annotation to the menu for the user's sake).  Perhaps we could turn that on its head though: when constructing the menu we could check if there exists an accel mapping for the action of a menuitem and add the annotation at that time...

Yeah, that pretty much mirrors my thoughts while writing this patch.

Looking at the GtkUIManager side of this, we did add a way to add explicit, 'freestanding' accelerators in addition to those tied to menuitems. We could do the same here - I just didn't feel like writing yet another parser...

Created attachment 202692 [details] [review]
another approach

Here is another version. This one adds api to set accelerators up independently from menus:

gtk_application_add_accelerator()
gtk_application_remove_accelerator()

It also relies more heavily on preexisting gtk machinery for accelerators.
We hardcode action_name/parameter combinations into the accel_path as
<Actions>/action_name/parameter, where parameter is a serialized GVariant.

GtkModelMenuItem simply sets this accel_path on itself, and relies on the accel map to be set accordingly.

GtkApplicationWindow loops over all accel map entries, and installs accelerators for each one that matches one of its actions.

With this approach, we gain

- accelerators work regardless if the menu is shown or not
- accelerators work for submenus that have not been instantiated yet
- it is possible to set up accelerators for actions that are not bound to menu items

Still to do:

- Need to listen for changes in both the action group and the accel map, and recreate the accelerators
- There is no way to parse accelerators out of xml
- It is currently not possible to set up multiple accelerators for the same action (except by giving it different parameters), but that would be fixable if desired.

Created attachment 202737 [details] [review]
combination

This patch keeps the add_accelerator api, but also extracts accels from the menus.
The extraction is currently done only once, at the time when the menu is set on the application.

is there a particular reason to keep the <Actions> namespace? if the user/developer needs namespaced action names, they can surely provide them themselves. if this is a unifying move to link accelerators and actions, why create a subset of all possible action names by requiring that they begin with <Actions> ?

(In reply to comment #4)
> Perhaps we could turn that on its head though:
> when constructing the menu we could check if there exists an accel mapping for
> the action of a menuitem and add the annotation at that time...

this is a particularly sane idea. i would note, however, that AFAICT it happens automatically in GtkMenus already, and given that, it should apply everywhere.

> is there a particular reason to keep the <Actions> namespace? if the
> user/developer needs namespaced action names, they can surely provide them
> themselves. if this is a unifying move to link accelerators and actions, why
> create a subset of all possible action names by requiring that they begin with
> <Actions> ?

The namespacing is enforced by existing GtkAccelMap code that we are reusing here.
It requires names of the form <...>/... 

> this is a particularly sane idea. i would note, however, that AFAICT it happens
> automatically in GtkMenus already, and given that, it should apply everywhere.

What do you mean by 'everywhere', exactly ?

If you look at my last patch, it simply pushes all the accel -> action mappings that we find in the menus as well as explicitly added ones into the accel map, and GtkApplicationWindow then installs accelerators for all of its actions that are found in the accel map. So the accels are really independent from the menus; we just use the menu xml as convenient way to establish the accel -> action mapping.

Reusing GtkAccelMap has the extra advantage that you can just as well use gtk_accel_map_load to load your accelerators from another file (and save changed accels with gtk_accel_map_save if you are so inclined...)

(In reply to comment #10)

> The namespacing is enforced by existing GtkAccelMap code that we are reusing
> here.
> It requires names of the form <...>/... 

unfortunate, and not really linked to any other kind of namespacing in the g* stack, but OK.

> > this is a particularly sane idea. i would note, however, that AFAICT it happens
> > automatically in GtkMenus already, and given that, it should apply everywhere.
> 
> What do you mean by 'everywhere', exactly ?

not sure. I detected a hint in Ryan's observation that picking the accels for a given application action might be harder if the menu is being built out of process (e.g. by some shell). i was just noting that "we do it already for menus, and there are new ways of displaying menus, they should continue to do it.
 
> If you look at my last patch, it simply pushes all the accel -> action mappings
> that we find in the menus as well as explicitly added ones into the accel map,
> and GtkApplicationWindow then installs accelerators for all of its actions that
> are found in the accel map. So the accels are really independent from the
> menus; we just use the menu xml as convenient way to establish the accel ->
> action mapping.

indeed. it would be nice, eventually, to provide only a single call to bind an accel to an action, thus removing the need to look for them in menus. gtk_application_add_accelerator() is as good a place as any to be the one call to take over this role.

Committed this to the branch now