migrate collection popover menu

[zisoft]

This is the first change on a popover menu as preparation for the gtk4 migration.

In gtk4, only the gtk_menu_button_...() functions are left, all other gtk_menu_...() functions are gone and need to be replaced.

According to the gtk4 migration guide, the way to go is to use menu models and actions.

An action entry consists of a title and the action name, both are strings. That means the action name needs to be linked to a callback function in some way, this is done by the ActionMap. Example:

  const GActionEntry entries[] = {
    { "clear", _menuitem_clear         },
    { "mode",  _menuitem_mode,  "(ii)" }
  };

  GSimpleActionGroup *group = g_simple_action_group_new();
  g_action_map_add_action_entries(G_ACTION_MAP(group), entries, G_N_ELEMENTS(entries), NULL);
  gtk_widget_insert_action_group(window, "collect", G_ACTION_GROUP(group));

Here we have two action items. The first item has the action name "clear" and is linked to the callback function _menuitem_clear(). No further parameters are passed to the callback function.

The second action item has the action name "mode" and is linked to the callback function _menuitem_mode(). It passes a tuple of two integer values to the function.

The action map is then bound to a widget for which it should work (gtk_widget_insert_action_group()). The chosen name here needs to be part of the action definition of the menu entries. This PR is for the collection module, so I have set the name to "collect".

Creating a menu model is simple: Create the model with g_menu_new() and add the items with g_menu_append() or one of the several other functions.

The menu items have a title and and the action name which should be executed when the menu item is clicked.

Passing parameters to the callback function is done via GVariant objects, which can hold all kinds of objects: Strings, numeric values, arrays, tuples, ... Passing pointers is not possible.

The parameters need to be encoded as string in the action part of a menu item. The callback function takes the parameters and decodes them.

GMenu *menu = g_menu_new();
g_menu_append(menu, "clear", "collect.clear");
g_menu_append(menu, "mode 456", "collect.mode((1, 456))");

The first item is bound to the action "clear" of the action group with the name "collect". No parameters are passed to the callback function.

The second item is bound to the action "mode" and passes a tuple of two integers 1 and 456 to the callback function.

Once this is understood, the creation of menus is quite simple. I have attached two small example programs, one for gtk3 and one for gtk4. Both should behave the same.

gtk3-test.c
gtk4-test.c

CSS styling: Popover menus have the popover selector with the class .menu, the menu items have the modelbutton selector. I have added a basic styling to the base theme darktable.css so it works in all other themes.

GtkPopoverMenu exists in both gtk3 and gtk4, I have tried to keep the code as compatible as possible.

In gtk3 we have gtk_popover_new_from_model(), in gtk4 this is gtk_popover_menu_new_from_model(). I have wrapped these function calls in a conditional compile flag in dt_gui_popover_menu_from_model(), located in src/gui/gtk.c, which works in both gtk versions. So no further change should be neccessary for the gtk4 migration.

@dterrahe @TurboGit: please review.

A quick search in the code base reveals to 38 calls to gtk_menu_new() which all need to be migrated this way. Can be somewhat challenging for 5.4

[TurboGit]

Not tested yet, just a quick review.

A quick search in the code base reveals to 38 calls to gtk_menu_new() which all need to be migrated this way. Can be somewhat challenging for 5.4

Maybe do the presets and styles ones are those have hierarchical menu and are the ones bugging on Wayland.

BTW, I just found out that the submenu do not open because they are on the right side of the screen. If I unmaximize the dt windows and move it to the left to let some room on the right the menu works:

[dterrahe]

For GTK3 it seems we can't remove the little "arrow" that connects the popup to the button it was called up from. This makes it look and behave less like a normal menu. My searches didn't turn up a way to change this look via settings or css.

It seems it is possible to build the contents of the menus without using the ActionModel approach that gtk pushes. Their app development philosophy is to set up you whole app as a series of supported actions that can be triggered in various ways, one being via menus. This is not how dt currently works internally and it would be quite a change, so I'm not convinced there is value in converting. Maybe we'd be better off building the content of the menus "manually" (using GtkModelButtons, maybe) rather than building a lot of scaffolding needed to then do it "automatically" via models. But I haven't looked into this too deeply.

This example doesn't yet show how this would work with variable length menus with submenus, so I can't really deduce how complex that would become. The presets menu would be a good/better testcase (besides the wayland issues that need to be solved there). Those menus currently also have extra functionality (like press-holding a menuitem to avoid the menu-tree being closed so you can quickly try adjacent presets as well). I have no idea as yet if the autogenerated model-based menus support this kind of functionality?

Tbh if I look at the gtk4-demo application and try out the first "application class" example, the main menu seems to be problematic to me. Submenus auto-open but don't auto-close so you end up with a bunch of them overlapping. I hope this is not a precursor of what we can expect after switching. We may be better off building our own hierarchical menu implementation ("bauhaus+submenus"). Not that this would be trivial, of course.

In any case, it would be my preference to build some "infrastructure" centrally and use that for the subset of functionality we need, rather than spreading out gactionmapentries and direct g_variant manipulation everywhere.

[TurboGit]

I have found a workaround for the issue of the closing popup. It is just a bit of CSS:

/* ??? workaround a wayland issue where the sub-menu in the presets
   closing when close to the right border of the screen. */
menuitem *
{
    padding: .05em;
}

I found that by reading that using standard GNOME Adwaita people have seen the issue fixed on other applications. After a bisect section to find what was wrong in our CSS I found out that a 0 margin was causing troubles.

Another Wayland issue fixed for 5.4. At this point I have a working Darktable, no more issue found (not saying that there is none :).

[zisoft]

For GTK3 it seems we can't remove the little "arrow" that connects the popup to the button it was called up from.

I have looked for that as well and came to the same conclusion.

Maybe we'd be better off building the content of the menus "manually" (using GtkModelButtons, maybe)

That was my first attempt and it turned out to be pretty straightforward to implement. Until I noticed that GtkModelButton does not exist in gtk4...

This example doesn't yet show how this would work with variable length menus with submenus, so I can't really deduce how complex that would become. The presets menu would be a good/better testcase

Agreed. I wanted to start with a simple menu to get a starting point for further discussion, hence my long description on how this all works. If the MenuModel/Action approach is the way to go, I would continue next with the presets menus. But of course I am completely open to any other approaches. My goal here was to come up with code which works in gtk3 and does not need to be touched again when migrating to gtk4.

[dterrahe]

Until I noticed that GtkModelButton does not exist in gtk4...

Yep, you seem to be right about that! The funny thing is, if you use gtkinspector to drill down into the gtk4-demo application, the contents of menus still seem to be built with GtkModelButtons. So I'm guessing they are still used internally but not exposed via the API.

My goal here was to come up with code which works in gtk3 and does not need to be touched again when migrating to gtk4.

Absolutely! Especially if we need to migrate to new menus in gtk3 because of wayland issues. But if @TurboGit indeed solved that problem, then maybe this should only be done during the full gtk4 migration, because it seems that the "backported" gtkpopupmenu has some problems that make it worse under gtk3.

It would be nice if we could somehow abstract out _process_variant_params into a utility function that can be used by all menus (to pass one, two or three gpointers), rather than having to redo this for each module. I foresee this getting copied over each time and being amended by less and less knowledgeable people. I myself find this whole new paradigm here a nuisance to get my head around...

[TurboGit]

But if @TurboGit indeed solved that problem, then maybe this should only be done during the full gtk4 migration

Yes, the issue is solved so no urgency. We are safe for the 5.4 release at this point. This is a good news as we won't let a good part of our users without Darktable as quite some distro have now dropped X11.

[dterrahe]

We don't need X11; we're fine with Xwayland. Or at least should be, if that's working correctly. My understanding has always been that no distribution has the intention of dropping Xwayland (since that would kill loads of legacy apps).

I know, I know, Xwayland is part of X11 so it's very easy to use incorrect terminology, but we can try. As far as I've understood so far, there's only a problem with one compositor under wayland. Unfortunately it is used by a major distro, but there isn't inherently a problem with wayland+Xwayland.

[TurboGit]

We don't need X11; we're fine with Xwayland. Or at least should be

Sure, but that's not working. See all reports and I experienced the same issues reported by many users. Darktable crashes, scrolling don't work, flying over lighttable is ineffective...

If one still want to use Xwayland it is possible with: GDK_BACKEND=x11 darktable

But I had better use my time to do a proper support of Wayland which is the only future :)