Created attachment 241493 [details] [review]
gremoteactiongroup: add platform data hooks

How is this different from the existing support for platform data in the _full variants ? An example of the intended uses would be good to understand this.

The problem is that with the existing system every single person who puts an action group on the bus has to deal with the platform_data themselves.  Since most of the time people don't want to directly interact with GDK (and in many cases can't) they do a tricky: get the current GApplication and directly call the ->before_emit() and ->after_emit() hooks on that.  The idea here is that hopefully this is a GtkApplication that does the 'right thing'.

GApplication itself does exactly this.  In fact, we have GRemoteActionGroup wrappers around our normal GActionGroup solely for the purpose of doing this.  This wrapper code has been more or less copy-paste duplicated between GApplication and GtkApplicationWindow.

The issue with this approach is that it's difficult to do (because it requires you to create a RemoteActionGroup wrapping your real action group).  Additionally, outside of GApplication itself, it is possible that maybe there is not a default GtkApplication in the current process.  Another other issue is that it's _excessively_ gross to do this.

This sort of global hook is ugly to be sure -- but this is a reflection of storing the timestamp as global state in the toolkit.  This will let us keep the ugly tucked into one tidy little corner...

Created attachment 242163 [details] [review]
gremoteactiongroup: add platform data hooks

Created attachment 242164 [details] [review]
gdkdisplay-x11: use gio's platform data hooks

before_emit and after_emit are called whenever an action is activated
from another process.  The handlers in this patch acquire the gdk lock
(because action invokation usually result in user-code being called) and
inject the invokation's timestamp into gdk.

add_platform_data is called before an action in another process is
called.  Gdk's handler adds the current time stamp into the platform
data.

This will eventually replace the vfuncs with the same names on
GtkApplication.

Review of attachment 242163 [details] [review]:

I notice that there are no changes to GApplication here....

We need a story for removing the hacked-up remoteactiongroups inside of GApplication and (particularly) GtkApplicationWindow.

::: gio/gremoteactiongroup-private.h
@@ +31,3 @@
+void        g_remote_action_group_after_emit        (GVariant *platform_data);
+
+GVariant *  g_remote_action_group_get_platform_data (void);

I get the impression that we may want to change this to a GHashTable* in the future, but it's internal so we can cross that bridge when we need to.

::: gio/gremoteactiongroup.c
@@ +240,3 @@
+ * @platform_data: a #GVariant of type "asv"
+ *
+ * The type of callback function used for subscribing to platform_data

This documentation is pretty weak.

But we don't really expect anybody to use this API unless they already know what they're doing...

Missing Since:, though.

@@ +257,3 @@
+ * the values when it is done with them.
+ *
+ * See also: g_remote_action_group_add_platform_data_hooks().

ditto on Since:

@@ +267,3 @@
+ *
+ * Adds handlers for adding and extracting platform data whenever a
+ * #GAction is activated across a process boundary.  Platform data

strictly, whenever an activation on a #GActionGroup occurs

also: this doesn't _strictly_ have to do with processes (although of course this is the only place it matters).  it really is about what happens between GActionGroup and GRemoteActionGroup.

@@ +293,3 @@
+  PlatformDataHooks *hooks;
+
+  if (platform_data_hooks == NULL)

use a static-allocated GQueue for this instead

Review of attachment 242164 [details] [review]:

This patch needs to modify GtkApplicationWindow to remove its private action group and to bump the GLib depend (otherwise installing the hooks isn't going to work).

::: gdk/x11/gdkdisplay-x11.c
@@ +174,3 @@
 
+static void
+before_platform_data_emit (GVariant *platform_data)

this function is named oddly....

@@ +184,3 @@
+      GdkDisplay *display;
+
+      display = gdk_display_get_default ();

this is highly global-statey and evil...

I think it's actually what we want in this case, though, considering the nature of the problem we're solving.


The other option is to add some user_data on the hooks... not sure we really want to do that, though.

It means that we should definitely mark the new API in GLib as (skip).

@@ +193,3 @@
+after_platform_data_emit (GVariant *platform_data)
+{
+  gdk_threads_leave ();

No explicit startup notification ACK here is going to regress some apps....

We'll have to match this with a fix to GtkWindow to do the right thing there.

Created attachment 246335 [details] [review]
gremoteactiongroup: add platform data hooks

Created attachment 246336 [details] [review]
GApplication: deprecate platform data vfuncs

Deprecates before_emit, after_emit, and add_platform_data vfuncs, since
toolkits can get the same functionality by installing global hooks with
g_remote_action_group_add_platform_data_hooks().

Review of attachment 246335 [details] [review]:

This turned out really nicely.  It's pretty much ready to go after a few minor nits.

::: gio/gremoteactiongroup.c
@@ +249,3 @@
+ * @platform_data is a #GHashTable mapping strings to #GVariants.  The
+ * hash table will call g_free() on the keys and g_variant_unref() on
+ * the values when it is done with them.

Please add a note about ensuring that you only add properly-sinked(sunk?) GVariants.

@@ +293,3 @@
+  hooks->add_platform_data = add_platform_data;
+
+  g_queue_push_head (&platform_data_hooks, hooks);

I find the way the queue is ordered to be anti-intuitive.... but this is just a nitpick since it's not API.

::: gio/tests/gremoteactiongroup-platformdata.c
@@ +3,3 @@
+
+static void
+setup_test_bus (GTestDBus     **bus,

please use the gdbus-sessionbus utilities for this like the rest of the tests

Review of attachment 246336 [details] [review]:

There is a complication here that you didn't deal with.  There are a bunch of direct calls to before/after_emit in the impl.  I guess those should be replaced with calls to the new functions on GRemoteActionGroup...

Otherwise, that big block of deleted code sure looks nice :)

Review of attachment 246336 [details] [review]:

::: gio/gapplication.c
@@ +637,3 @@
+      class->add_platform_data != g_application_real_add_platform_data)
+    {
+      g_warning ("GApplication: before_emit, after_emit, and add_platform_data are deprecated. "

note that this will warn on every single GtkApplication in existence if the user installs a new GLib but not new Gtk

maybe we should make a special exception: if we find that the class of the instance in question is a subclass of a class with the name "GtkApplication" and its handlers for these vfuncs are equal to the ones on this "GtkApplication" class, then we can skip the warning (since we already fixed gtk, so we don't need more nags).

Created attachment 246441 [details] [review]
gremoteactiongroup: add platform data hooks

Clarify that the hash table takes sunk variants, reverse the order of the
queue, and use gdbus-sessionbus in tests.

Created attachment 246443 [details] [review]
GApplication: deprecate platform data vfuncs

Turn direct calls to {before,after}_emit into
g_remote_action_group_{before,after}_emit.

Don't warn when application is a GtkApplication.

Review of attachment 246441 [details] [review]:

Thanks.

Review of attachment 246443 [details] [review]:

::: gio/gapplication.c
@@ +637,3 @@
+      class->add_platform_data != g_application_real_add_platform_data)
+    {
+      if (!g_type_is_a (G_OBJECT_TYPE (application), g_type_from_name ("GtkApplication")))

This is not good enough...

First, I'm not sure what will happen here if g_type_from_name ("GtkApplication") fails.

Second, This will suppress warnings for all GtkApplications (ie: pretty much all GApplications).  Rather, you should get the class vtable for GtkApplication and compare the virtual pointers it has for these three functions to the one in the user's vtable (which my itself _be_ GtkApplication).  If you find them to be equal, don't complain (but still install the compat hook).

Created attachment 246530 [details] [review]
GApplication: deprecate platform data vfuncs

Good point, thanks Ryan.

Review of attachment 246441 [details] [review]:

Probably will take a different approach here...

I was pointed here after wontfixing bug 722553, and I believe this is the wrong approach, because it conflates in GRemoteActionGroup two very different operations.
One the one hand, you have the receiver side (before_emit/after_emit), on the other hand you have the sender side (add_platform_data).
So far, GRemoteActionGroup only dealt with the sender side, and indeed it was implied by the name and purpose that it was only for remote actions, so anything receiver related is not necessary.

Having cleared that, I don't see what is the benefit of a add_platform_data() hook over manually building the platform data and passing it down the activate() call, using the existing API.

Platform data may not be the same across all platforms (OS X won't use timestamps, Ryan wants to do something different for Wayland as well), so we shouldn't add such assumptions to our API.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/691.