"reversible"

[Ryan Lortie <desrt desrt ca>]

hi,

One of the more annoying aspects of using GMenuModel from an application 
that supports plugins is that adding items to a menu and being able to 
remove them again in the future is not well-supported.

GMenuModel allows the use of empty sections as a (superior) stand-in for 
the prior placeholder functionality of GtkUIManager and I've been 
recommending them to people.  I always get asked how to remove the items 
from the menu again, however, and I rarely have a good answer for that.

It's usually something along the lines of "tag it with an attribute that 
you will recognise and iterate/remove" or "add a subsection and then 
iterate over the section to find your subsection (by pointer ID) and 
remove it".

The GtkUIManager approach to this is to use integer tags to facilitate 
backing out UI additions.

Although I've often considered adding this, it would probably not work 
great with GMenuModel.  There is no merging (in the GtkUIManager sense) 
so you have to manually add your several menu items -- and would have to 
keep around a list of numbers for later removal.

I've found myself musing in the past about some sort of start() and 
end() API along these lines:

  id = g_menu_start_adding (menu);
  g_menu_append (menu, ...);
  g_menu_append (menu, ...);
  g_menu_append (menu, ...);
  g_menu_stop_adding (menu);


then later:

  g_menu_remove_additions (menu, id);


This would give something close to GtkUIManager but it's really ugly.  I 
don't particularly care for storing state on objects in this way.

I've also thought about this sort of approach:

  g_menu_add_with_removal_id (menu, id, ...);

then later:

  g_menu_remove_by_id (menu, id);

but this sort of thing turns into an API combinatorial-explosion-style 
nightmare.


One place that we haven't been too shy about storing state in the past 
is in thread-local storage.  We have at least thread-local default main 
contexts as well as thread-local cancellables that can be directly 
manipulated by the user.

This leads to this idea:

  id = g_menu_push_thread_removal_id ();
  g_menu_add (menu, ...);
  g_menu_add (menu, ...);
  g_menu_pop_thread_removal_id ();

  g_menu_remove_by_id (id);


But then I got to thinking that plugins add menu items, actions and 
possibly many other things.  It would be silly to have this for everything.

This led me to the idea of some object with a name like GReversible 
(although I don't think the name is a great fit for the same reason that 
GCancellable is slightly misnamed).

Certain operations could advertise themselves as "reversible".  Adding 
items to a menu or actions to a GActionMap are obvious candidates.  Then 
a plugin could have something like so:

struct _PluginState
{
  GReversible *reversible;
};

void add_window (GtkApplication *app,
                 GtkWindow      *window,
                 PluginState    *state)

{
  g_reversible_push (state->reversible);
  g_action_map_add_action_entries (G_ACTION_MAP (window),
                                   plugin_win_actions,
                                   G_N_ELEMENTS (plugin_win_actions),
                                   window);
  g_reversible_pop ();
}

void plugin_init (PluginState *state,
                  GtkApplication *app)
{
  state->reversible = g_reversible_new ();

  forall (win in windows) { add_window (state); }
  app.connect ("window-added", add_window, state);

  g_reversible_push (state->reversible);

  g_action_map_add_action_entries (G_ACTION_MAP (app),
                                   plugin_app_actions,
                                   G_N_ELEMENTS (plugin_app_actions),
                                   app);

  menu = gtk_application_get_app_menu (app);
  add_stuff_to_menu (menu);

  g_reversible_pop (state->reversible);
}

void plugin_remove (PluginState *state)
{
  g_reversible_reverse (state->reversible);
  g_clear_object (&state->reversible);
}


It's pretty clear that the functionality of the GReversible object is 
quite close to that of GCancellable.  It might make sense to share them. 
 Resetting the object probably makes even less sense here than it does 
for resetting a cancellable.

One thing that doesn't make sense would be to share the thread-local 
stack.  The meaning of a reversible and a cancellable operation are very 
much separate and should be kept so.

We could use g_reversible_enter() that returns an integer tag, but this 
prevents us from having multiple groups of operations able to be 
reversed by a single reversible.  We could fix that by allowing the user 
to specify an already-existing reversible ID.

On balance, I'd prefer not to use GCancellable -- probably creating 
GReversible or using integer IDs makes most sense.


The trickiest bit of this proposal, in my opinion, is managing the 
transition of classes from non-reversible to reversible.  Someone may be 
surprised to find that one day:

  g_reversible_enter();
  g_hash_table_insert();
  g_reversible_leave();

  g_reversible_reverse();

this code actually removes their inserted item from the hashtable (I 
know this is a ridiculous case but there could be other less-ridiculous 
ones).

This is a sort of transition that we've managed well in the past, in my 
opinion.  Objects have become thread-default-mainloop-aware in the past 
without too much incompatible fallout.  I think it would probably be 
prudent to do a full review of all potentially-reversible APIs in GIO 
and decide which ones will be reversible at the point that we introduce 
the new API.  In any case, the documentation for GReversible should 
strongly suggest not pushing a reversible for long lengths of time.


When it comes to menus (and actiongroups) I'd probably manage the 
semantics of reversibles in approximately this way:

 - nesting is supported

 - if a reversible is pushed then you may only call g_menu_remove() for
   items that were added while under that reversible (ie: removal is
   non-reversible)

 - reversing a reversible will reverse actions done even if another
   reversible was nested at the time.  example:

   enter (a);
   enter (b);
   add();
   leave(b);
   leave(a);

   reverse(a);    // would reverse the add

 - removing items pushed while in a reversible from outside of that
   reversible would work as usual but would not be recommended


Would this API make your life easier?  Could it be better?

Cheers