(In reply to comment #0)
> - need to keep an eye on thread-safety.

I was actually thinking it would be more appropriate to add some comments along the lines of "you should not even think about using this from anything other than the default thread".  The reason for that is because of the change signals: they need to be delivered to a particular thread.  That means that when the change signal fires, it will be in a particular thread.

I could go to quite a lot of effort to try to support having parallel trees of menus for dispatch of signals from different main context, but from a practical standpoint, this all ends up being used only from the main context (since it's somewhat UI-centric).

> - do we standardize on 'org.gtk.' interface names now ?

Seems pretty reasonable for things in GLib.  We have GActionGroup as a precedent.

> - even if we consider the dbus interfaces for actiongroups and
>   menus to be private, we should at least have some internal
>   docs for them. The stuff in
> http://people.gnome.org/~ryanl/valmynd/_literal_ca_desrt_valmynd_menu_literal.html#_conceptual_overview
>   would be good to add somewhere

Agreed.
 
> - your use of ':' and '::' in parsing various strings seems to indicate
>   that these should be avoided in action names and other strings ? These
>   restrictions should be spelled out somewhere. I don't even see why the 
>   initial ':' for links is really needed though - can't you just infer that
>   from either the name (section or submenu) or from the variant type ?

Docs and maybe some g_return-style checks.  I think alphanum plus "-" is appropriate.

>   g_action_group_export
>   g_action_group_stop_exporting
>   g_action_group_is_exported

My problem with this is that this is something that is D-Bus specific and the API makes it look like a core feature of the action group.  I want to stress that it is different.  Perhaps 'exporter' is not the way to do that, but rather 'dbus':


So perhaps:

  g_dbus_action_group_export (...)

and

  g_dbus_action_group_import (...)

I sort of like the way that looks, actually.

> - What about exporting on other platforms ? Any thoughts about that ?

Someone will have to look into what is required to support this on the mac.

> GMenuModel
> 
> - long desc needs to have _much_ more background
>   - what is this about ? (exporting menus from applications, cite examples: 
>     global menus, appmenus, docks, etc). I've added some of my own
>     understanding there now, but more is needed.
>   - explain your conceptual model of menus (items, submenus,
>     sections, etc) and how the api maps to it
>   - what are links ?
>   - can models represent section or submenus, depending
>     on context ?
>   - what attributes does an item have ?
>   - how is one supposed to use this ?

I feel that the valmynd documentation does a pretty good job of answering these questions in the 'conceptual overview' section.  I intend to merge that here.
 
> - it is not entirely clear to me what 'mutable' is supposed to mean exactly.
>   Does it mean 'I can't change it' or does it mean 'This won't change' ?
>   E.g I was surprised that GMenuProxy is mutable.

GMenuModel is always a read-only interface.  By that, I merely mean "you may not write to it".  That does not mean that it never changes.

Think like GObject properties: if we mark them READABLE, but not WRITABLE, it merely means that g_object_set() can not be called for that property.  It doesn't mean the property never changes or emits the "notify" signal -- that's mutability.

is_mutable() is basically a way to find out if the "items-changed" signal will ever be emitted.

I had a thought that it might make sense to change this to a get_flags() API, but I have a hard time imagining what other flags may be appropriate.

> - seems the api is all in terms of positions. but
>   apis like append do not return positions. Should they ?

This would not be difficult to add.

>   Should there be some 'find_position' api ?

I doubt it.

> Also, I could
>   see positions getting somewhat finicky when you have sections
>   that do visually appear inside the menu, but for position counting
>   purposes only contribute 1...

I think the programmer needs to keep conceptual clarity in mind here and I don't think it's a problem.  People get by with boxes inside of boxes with relatively little pain.
 
> - how does the 'action' attribute identify actions ? that is not
>   really explained in sufficient detail, or at all even

And not yet fully decided to be honest.  I'd like to have a discussion about that.  I think it should be along the lines of 'app.quit' and 'doc.save' and so on to invoke the named action on the named actiongroup.

> - needs to explain that the parser is meant for parsing
>   fragments, with an example

Could possibly also use some convenience API for the case where you just have a string you want to parse...

> - same comment as for GActionGroupExporter: 'exporter' appears
>   like a phantom object in the docs. Could just be
>   g_menu_model_export
>   g_menu_model_stop_exporting
>   g_menu_model_is_exported

g_dbus_menu_export() and g_dbus_menu_import()?

> - can I get a proxy for a menu other than the one with id 0 ?

No.

(In reply to comment #1)
> (In reply to comment #0)
> > - need to keep an eye on thread-safety.
> 
> I was actually thinking it would be more appropriate to add some comments along
> the lines of "you should not even think about using this from anything other
> than the default thread".  The reason for that is because of the change
> signals: they need to be delivered to a particular thread.  That means that
> when the change signal fires, it will be in a particular thread.
>
> I could go to quite a lot of effort to try to support having parallel trees of
> menus for dispatch of signals from different main context, but from a practical
> standpoint, this all ends up being used only from the main context (since it's
> somewhat UI-centric).

Right now, you just seem to be using g_idle_add, so all the signals end up in the default context. I don't really see how that lets use break the rule that all global data structures shall be properly locked.


> 
> My problem with this is that this is something that is D-Bus specific and the
> API makes it look like a core feature of the action group.  I want to stress
> that it is different.  Perhaps 'exporter' is not the way to do that, but rather
> 'dbus':
> 
> 
> So perhaps:
> 
>   g_dbus_action_group_export (...)
> 
> and
> 
>   g_dbus_action_group_import (...)
> 
> I sort of like the way that looks, actually.

I like this too.

> 
> I feel that the valmynd documentation does a pretty good job of answering these
> questions in the 'conceptual overview' section.  I intend to merge that here.

Sounds great. Although it seemed to me that some of the details mentioned in the valmynd docs are not actually reflected in the implementation (e.g. the docs say something about sending action state along with menus for the initial dump - that doesn't seem to happen currently).

> 
> is_mutable() is basically a way to find out if the "items-changed" signal will
> ever be emitted.

Thats a very clear statement - would be good to put that in the docs. Together with the fact that menus can go from mutable to immutable (but not the other way, obviously).


> I think the programmer needs to keep conceptual clarity in mind here and I
> don't think it's a problem.  People get by with boxes inside of boxes with
> relatively little pain.

We do expect application programmers to mainly use some higher-level GtkApplication api instead of GMenuModel, though ? Or do you expect that people will put menu models together manually ?

> > - how does the 'action' attribute identify actions ? that is not
> >   really explained in sufficient detail, or at all even
> 
> And not yet fully decided to be honest.  I'd like to have a discussion about
> that.  I think it should be along the lines of 'app.quit' and 'doc.save' and so
> on to invoke the named action on the named actiongroup.

Do our action groups even have names ? 
On the dbus level, 'exported at the same path' seems ok to me. 

> 
> g_dbus_menu_export() and g_dbus_menu_import()?

Sounds good to me.

Here's another question I had while looking at your api:

Why do you have this general link iteration code, and code to query links by type ?
It seem that with your conceptual model of menus, each item can only ever have a single link anyway...

> We do expect application programmers to mainly use some higher-level
> GtkApplication api instead of GMenuModel, though ? Or do you expect that people
> will put menu models together manually ?
> 

After some minor modifications to GtkBuilder, I picture it working something like this:

<gtk-builder-file>
  ...
  ...
  <menu id='my-main-menu'>
    <submenu label='_File'>
      <section>
        <item label='_New' action='app.new'/>
        ...
      </section>
      <section id='recent-documents'/>
      <section>
        <item label='_Quit' action='app.quit'/>
      </section>
    </submenu>

    <submenu label='_Edit'>
      ...
    </submenu>
    ...
  </menu>
  ...
</gtk-builder-file>


with code something like this:

{
  GMenu my_menu = builder.get_object ("my-main-menu");
  app_window.set_menu (my_menu);

  GMenu recent = builder.get_object ("recent-documents");
  recent.append ("mydoc1.txt", "openfile::file://home/desrt/mydoc1.txt");
  recent.append ("mydoc2.txt", "openfile::file://home/desrt/mydoc2.txt");
  ...
}

Interesting to see that you have mnemonics there ... how do expect those to work in remote cases ? I guess we'll just filter those out for remote display.

It depends on the implementation.  Unity will probably use passive grabs and a handy XKB extension to notify when the alt key is pressed (for deciding to draw the underline or not).

Passive grabs are really wrong for this.
No comment on shipping X protocol forks - I'm just going to have to ignore Ubuntu bugs altogether...

I've pushed a very quick implementation of GtkBuilder support to the gtk branch.

One thing I haven't seen mentioned in the docs or code is extensibility - any thoughts about allowing other attributes like, say, icons ? 

Tangentially related: Are there any a11y concerns here ?

(In reply to comment #1)

> > - even if we consider the dbus interfaces for actiongroups and
> >   menus to be private, we should at least have some internal
> >   docs for them. The stuff in
> > http://people.gnome.org/~ryanl/valmynd/_literal_ca_desrt_valmynd_menu_literal.html#_conceptual_overview
> >   would be good to add somewhere
> 
> Agreed.

The valmynd doc integration is done on the wip/menus-rebase3 branch.

> > - your use of ':' and '::' in parsing various strings seems to indicate
> >   that these should be avoided in action names and other strings ? These
> >   restrictions should be spelled out somewhere. I don't even see why the 
> >   initial ':' for links is really needed though - can't you just infer that
> >   from either the name (section or submenu) or from the variant type ?
> 
> Docs and maybe some g_return-style checks.  I think alphanum plus "-" is
> appropriate.

I've added checks in the wip/menus-rebase3 branch. I ended using the same restrictions that we have for gsettings keys: only lowercase ascii alpha, digits and '-', with some additional restrictions.


> So perhaps:
> 
>   g_dbus_action_group_export (...)
> 
> and
> 
>   g_dbus_action_group_import (...)

I've renamed the exporter functions in wip/menus-rebase3 now. I ended up using slightly different names.


Here is what I consider the remaining merge blockers:

- Detect fallback situations, and have a way for GtkApplication to construct a menu in-process in that case

This could be done by adding a org.gtk.Menus.Register function that the shell would call upon first seeing the app, maybe. Or it could live as a kludge in gtk, simply looking for the shell/unity as window manager. Guess what my preference is... 

- Specify how menus and actions are tied together and document where things are exported

I think we should have a 'D-Bus specifics' section in the GApplication docs that explains some details of D-Bus use in GApplication.

- What about quit ?

Do we need a g_application_quit() ? Does that interfere with use counts ? Do we make it easy to export that as an action, or do we export it automatically ?

In recent irc discussion, I admitted that 'quit' should not block the merge.

One thing I forgot though, and that I still think needs addressing is thread-safety.

One more serious problem I found while adding tests is

 * GMenuProxyItem is just a pair of hashtables, one for the attributes
 * and one for the links of the item (mapping strings to
 * other GMenuProxy instances). XXX reconsider this since it means the
 * submenu proxies stay around forever.....

This is actually causing problems where all submenus are leaking from one test to the next and cause wrong menus to be reported. After the /gmenu/dbus/roundtrip test has completed, there's some 500 (!) menu proxies leftover...

Created attachment 202287 [details] [review]
some thread safety

Here is a patch that tried to address thread-safety in gactiongroupexporter.c.
It is not quite perfect - the signal handlers need to keep the exporter object alive while they are running.

Created attachment 202344 [details] [review]
some more thread safety

trying to do the same for the menu exporter

So there's this really annoying situation with GObject in general -- it's never clear where you should expect signals to be delivered.  Other object systems have support for automatically routing dispatch via a particular main context.

For the importer classes, it seems clear to me that the correct way to dealing with threadsafety is that we should create the importer relative to the default main context *at the time of creation*.  GObject doesn't have an option for cross-thread signal dispatch, so we need to have an object per maincontext.  That's annoying, but OK.

On the other side, for the action exporter, we surely must be able to assume that each action group that we are exporting is also emitting signals relative to some particular main context.

Menus are slightly more complicated because they can contain each other.  One could imagine some insane situation in which different menus could link to each other from across main-context ownership boundaries.

No matter what, our interaction with the outside world is limited to two things:

  - querying the groups/menus in response to change signals from that object

  - sending DBus signals

GDBusConnection is perfectly happy to have us hitting it from whatever thread we please.

Querying the groups/menus should also be completely safe, since if a signal is being emitted on that object from a particular thread it is obviously safe to call back into that same object on the same thread.

All that's left is the possibility (at least for the menu case) that two different menus are emitting signals at the same time on different threads.  Since we're okay with respect to our interactions with the rest of the world, all we need to do is use a lock for ourselves.



I really wonder if this is needed at all, though.  What is the usecase for accessing actiongroups and/or menus from multiple threads at the same time?

Accessing the same actiongroup or menu from multiple threads at the same time is not a usecase. The glib docs are clear on the point that you are expected to do your own locking around individual data structures.

What is a concern is access to different menus from from multiple threads leading to concurrent modifications of the shared data structures. The tower of hash tables there is pretty complicated, and I'm sure I have not nailed the problem in my first attempt...

Both the action group exporter and menu exporter can now export multiple actiongroups/menumodels in separate threads without cross-talk.