Created attachment 321591 [details] [review]
core: Refactor startup notification into a separate object

This is kind of in a middle ground at the moment. Even though it
handles sequences not coming from libsn, they're added nowhere at
the moment, we'll rely on the app launch context being in the x11
side at the moment.

Also, even though we do create internal sequence objects, we keep
exposing SnStartupSequences to make gnome-shell happy, we could
consider making this object "public" (and the sequence objects with
it), things stay private at the moment.

Created attachment 321592 [details] [review]
wayland: Update gtk-shell protocol file to v3

Add a gtk_shell.set_startup_id request, so the application can communicate
to the compositor the startup id that it received through the
DESKTOP_STARTUP_ID envvar, or other means.

Created attachment 321593 [details] [review]
wayland: Implement gtk-shell v3

Implement the gtk_shell.set_startup_id request, so that the ID is
removed from the sequences list, and feedback updated accordingly.

Review of attachment 321591 [details] [review]:

Other than the comments below this seems fine

::: src/core/screen.c
@@ -2743,2 @@
 /**
  * meta_screen_get_startup_sequences: (skip)

So, this function's implementation was inside the #ifdef but its prototype is public in meta/screen.h ?

I think it's safe to get rid of it.

::: src/core/startup-notification.c
@@ +109,3 @@
+#define META_STARTUP_NOTIFICATION_SEQUENCE_GET_CLASS(o) (G_TYPE_INSTANCE_GET_CLASS ((o), META_TYPE_STARTUP_NOTIFICATION_SEQUENCE, MetaStartupNotificationSequenceClass))
+
+G_DEFINE_TYPE (MetaStartupNotification,

could use G_DECLARE_*_TYPE() to reduce the boilerplate

@@ +264,3 @@
+#ifdef HAVE_STARTUP_NOTIFICATION
+static void
+meta_startup_notification_sequence_complete (MetaStartupNotificationSequence *seq)

should be out of the #ifdef

@@ +378,3 @@
+
+static void
+meta_startup_notification_add_sequence_internal (MetaStartupNotification         *sn,

should be out of the #ifdef

@@ +445,3 @@
+
+static void
+meta_startup_notification_ensure_timeout (MetaStartupNotification *sn)

should be out of the #ifdef

@@ +507,3 @@
+
+  sn_monitor_context_unref (sn->sn_context);
+  G_OBJECT_CLASS (meta_startup_notification_parent_class)->finalize (object);

missing the #ifdef and the other variables: sn_display, startup_sequences and startup_sequence_timeout

@@ +648,3 @@
+  sn->startup_sequences = NULL;
+  sn->startup_sequence_timeout = 0;
+#endif

the #endif should be 2 lines up ?

@@ +721,3 @@
+  for (l = sn->startup_sequences; l; l = l->next)
+    {
+#ifdef HAVE_STARTUP_NOTIFICATION

seems like there's no need to leave the loop out of the #ifdef

Review of attachment 321592 [details] [review]:

++

Review of attachment 321593 [details] [review]:

I know I'm very late in the process, but you should set startup_id on the window being started to properly apply startup properties (most importantly workspace), and that means this request should be on gtk_surface not gtk_shell.
If you don't do that, legacy mapping of startup sequences will still kick in and find the window based on wm_class (app_id on wayland), but that's unreliable and does not match what X11 does.

Review of attachment 321593 [details] [review]:

right

(In reply to Rui Matos from comment #4)
>   * meta_screen_get_startup_sequences: (skip)
> 
> So, this function's implementation was inside the #ifdef but its prototype
> is public in meta/screen.h ?
> 
> I think it's safe to get rid of it.

Nevermind, I found where it's used in gnome-shell.

(In reply to Giovanni Campagna from comment #6)
> Review of attachment 321593 [details] [review] [review]:
> 
> I know I'm very late in the process, but you should set startup_id on the
> window being started to properly apply startup properties (most importantly
> workspace), and that means this request should be on gtk_surface not
> gtk_shell.
> If you don't do that, legacy mapping of startup sequences will still kick in
> and find the window based on wm_class (app_id on wayland), but that's
> unreliable and does not match what X11 does.

This probably needs some more thinking than this approach, in gtk+ this is per-display API, so we can't tweak right away to provide the window being shown. I also think there's kind of an egg/chicken problem, this request is meant to happen before the window is shown, but having a gtk_surface means you probably already have an xdg_surface too, so the surface is technically visible.

I guess a request like set_startup_id (wl_surface, str) would be an intermediate approach, still not something we can honor in gtk+...

Created attachment 321613 [details] [review]
core: Refactor startup notification into a separate object

This is kind of in a middle ground at the moment. Even though it
handles sequences not coming from libsn, they're added nowhere at
the moment, we'll rely on the app launch context being in the x11
side at the moment.

Also, even though we do create internal sequence objects, we keep
exposing SnStartupSequences to make gnome-shell happy, we could
consider making this object "public" (and the sequence objects with
it), things stay private at the moment.

Couldn't take these functions of the ifdef... they're otherwise unused if SN is disable, which -Werror=unused-function takes quite seriously.

Review of attachment 321613 [details] [review]:

otherwise looks fine

::: src/core/startup-notification.c
@@ +41,3 @@
+typedef struct _MetaStartupNotification MetaStartupNotification;
+typedef struct _MetaStartupNotificationSequence MetaStartupNotificationSequence;
+typedef struct _MetaStartupNotificationSequenceClass MetaStartupNotificationSequenceClass;

these aren't needed

@@ +96,3 @@
+};
+
+GType meta_startup_notification_get_type (void) G_GNUC_CONST;

this should be

#define META_TYPE_STARTUP_NOTIFICATION_SEQUENCE (meta_startup_notification_sequence_get_type ())

@@ +101,3 @@
+                          meta_startup_notification_sequence,
+                          META, STARTUP_NOTIFICATION_SEQUENCE,
+                          GObject)

this and the previous line should go above the Private and Class struct declarations

@@ +106,3 @@
+               meta_startup_notification,
+               G_TYPE_OBJECT)
+G_DEFINE_TYPE (MetaStartupNotificationSequence,

G_DEFINE_TYPE_WITH_PRIVATE

@@ +125,3 @@
+static GParamSpec *seq_x11_props[N_SEQ_X11_PROPS];
+
+GType meta_startup_notification_sequence_x11_get_type (void) G_GNUC_CONST;

should be

#define META_TYPE_STARTUP_NOTIFICATION_SEQUENCE_X11 (meta_startup_notification_sequence_x11_get_type ())

@@ +134,3 @@
+G_DEFINE_TYPE (MetaStartupNotificationSequenceX11,
+               meta_startup_notification_sequence_x11,
+               meta_startup_notification_sequence_get_type())

could use the macro here for consistency

Thanks for the review! pushed with these last suggested changes.

Attachment 321592 [details] pushed as 3729e59 - wayland: Update gtk-shell protocol file to v3
Attachment 321593 [details] pushed as 3f60a2e - wayland: Implement gtk-shell v3
Attachment 321613 [details] pushed as 56beedf - core: Refactor startup notification into a separate object

(In reply to Carlos Garnacho from comment #9)
> (In reply to Giovanni Campagna from comment #6)
> > Review of attachment 321593 [details] [review] [review] [review]:
> > 
> > I know I'm very late in the process, but you should set startup_id on the
> > window being started to properly apply startup properties (most importantly
> > workspace), and that means this request should be on gtk_surface not
> > gtk_shell.
> > If you don't do that, legacy mapping of startup sequences will still kick in
> > and find the window based on wm_class (app_id on wayland), but that's
> > unreliable and does not match what X11 does.
> 
> This probably needs some more thinking than this approach, in gtk+ this is
> per-display API, so we can't tweak right away to provide the window being
> shown. I also think there's kind of an egg/chicken problem, this request is
> meant to happen before the window is shown, but having a gtk_surface means
> you probably already have an xdg_surface too, so the surface is technically
> visible.
> 
> I guess a request like set_startup_id (wl_surface, str) would be an
> intermediate approach, still not something we can honor in gtk+...

The surface is not visible until the first commit, so you can definitely have the request before it is shown.
Also gtk+ has to implement _NET_STARTUP_ID on x11, which is a window property, so there cannot be any gtk limitation.

(In reply to Giovanni Campagna from comment #14)
> (In reply to Carlos Garnacho from comment #9)
> > (In reply to Giovanni Campagna from comment #6)
> > > Review of attachment 321593 [details] [review] [review] [review] [review]:
> > > 
> > > I know I'm very late in the process, but you should set startup_id on the
> > > window being started to properly apply startup properties (most importantly
> > > workspace), and that means this request should be on gtk_surface not
> > > gtk_shell.
> > > If you don't do that, legacy mapping of startup sequences will still kick in
> > > and find the window based on wm_class (app_id on wayland), but that's
> > > unreliable and does not match what X11 does.
> > 
> > This probably needs some more thinking than this approach, in gtk+ this is
> > per-display API, so we can't tweak right away to provide the window being
> > shown. I also think there's kind of an egg/chicken problem, this request is
> > meant to happen before the window is shown, but having a gtk_surface means
> > you probably already have an xdg_surface too, so the surface is technically
> > visible.
> > 
> > I guess a request like set_startup_id (wl_surface, str) would be an
> > intermediate approach, still not something we can honor in gtk+...
> 
> The surface is not visible until the first commit, so you can definitely
> have the request before it is shown.

Point, I proposed a startup notification draft in wayland-devel, would be great to observe this there. For this basic support feels like kind of a rush though.

> Also gtk+ has to implement _NET_STARTUP_ID on x11, which is a window
> property, so there cannot be any gtk limitation.

In X11, this obviously involves Windows :), we don't tell which though, this is the function sending the _NET_STARTUP_INFO message:
https://git.gnome.org/browse/gtk+/tree/gdk/x11/gdkdisplay-x11.c#n2099

Notice how we XCreateWindow, send, and destroy.

Also, note the API call is gdk_display_notify_startup_complete (GdkDisplay*, gchar*), this probably means API changes.

(In reply to Carlos Garnacho from comment #9)
> This probably needs some more thinking than this approach, in gtk+ this is
> per-display API

gdk_window_set_startup_id doesn't count?

(In reply to Jasper St. Pierre from comment #17)
> (In reply to Carlos Garnacho from comment #9)
> > This probably needs some more thinking than this approach, in gtk+ this is
> > per-display API
> 
> gdk_window_set_startup_id doesn't count?

Not quite AFAICT... that API is completely optional (for apps actually doing multi-window management), otherwise the automatic treatment in gdk done by gdk_display_notify_startup_complete() suffices. I could only spot nautilus using this API.

That said, I agree this probably should be done at the surface level for wayland, I just wonder how though. Trying to pick the startup ID on the first window created in gdk/wayland doesn't strike me as a fine choice. Doing this right might bring some further changes than we can compatibly do AFAICS (eg. gtk_window_set_auto_startup_notification gets in the middle).

It's not optional -- it's used by GTK+ as part of startup notification.

https://git.gnome.org/browse/gtk+/tree/gtk/gtkwindow.c#n2476

gtk_window_set_startup_id() is called nowhere by gtk+ :)

That's half true.

Indeed, the right function is gdk_x11_display_set_startup_notification_id, which sets tto the leader window. So in the X11 protocol it's a per-window or per-group property, then if you set it to a group mutter moves it to each window in the group.
In wayland there are no window groups, so it should be set on all windows. You don't need API changes, just iterate through the windows. mutter will ignore it if you set the startup id on a window that was already mapped.