After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 762268 - Add basic startup notification support for wayland
Add basic startup notification support for wayland
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: wayland
git master
Other Linux
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
Depends on:
Blocks:
 
 
Reported: 2016-02-18 15:33 UTC by Carlos Garnacho
Modified: 2016-02-20 21:58 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
core: Refactor startup notification into a separate object (39.52 KB, patch)
2016-02-18 15:34 UTC, Carlos Garnacho
none Details | Review
wayland: Update gtk-shell protocol file to v3 (1.64 KB, patch)
2016-02-18 15:34 UTC, Carlos Garnacho
committed Details | Review
wayland: Implement gtk-shell v3 (2.28 KB, patch)
2016-02-18 15:35 UTC, Carlos Garnacho
committed Details | Review
core: Refactor startup notification into a separate object (39.34 KB, patch)
2016-02-18 19:53 UTC, Carlos Garnacho
committed Details | Review

Description Carlos Garnacho 2016-02-18 15:33:39 UTC
In wip/garnacho/wayland-startup-notification I added the basics to cover applications being launched from the shell, so apps can reply with the DESKTOP_STARTUP_ID we bake for the X11 side.

Otherwise, this is still quite dependent on the X11 side of things, both on libsn, and us using GdkAppLaunchContext (i.e. on the X11 side) to launch apps.
Comment 1 Carlos Garnacho 2016-02-18 15:34:53 UTC
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.
Comment 2 Carlos Garnacho 2016-02-18 15:34:59 UTC
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.
Comment 3 Carlos Garnacho 2016-02-18 15:35:04 UTC
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.
Comment 4 Rui Matos 2016-02-18 17:05:39 UTC
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
Comment 5 Rui Matos 2016-02-18 17:12:39 UTC
Review of attachment 321592 [details] [review]:

++
Comment 6 Giovanni Campagna 2016-02-18 17:39:53 UTC
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.
Comment 7 Rui Matos 2016-02-18 17:42:42 UTC
Review of attachment 321593 [details] [review]:

right
Comment 8 Rui Matos 2016-02-18 17:45:06 UTC
(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.
Comment 9 Carlos Garnacho 2016-02-18 18:35:33 UTC
(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+...
Comment 10 Carlos Garnacho 2016-02-18 19:53:13 UTC
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.
Comment 11 Carlos Garnacho 2016-02-18 19:55:13 UTC
Couldn't take these functions of the ifdef... they're otherwise unused if SN is disable, which -Werror=unused-function takes quite seriously.
Comment 12 Rui Matos 2016-02-19 14:27:21 UTC
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
Comment 13 Carlos Garnacho 2016-02-19 16:46:14 UTC
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
Comment 14 Giovanni Campagna 2016-02-19 17:38:47 UTC
(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.
Comment 15 Carlos Garnacho 2016-02-19 17:59:33 UTC
(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.
Comment 16 Carlos Garnacho 2016-02-19 18:02:06 UTC
Also, note the API call is gdk_display_notify_startup_complete (GdkDisplay*, gchar*), this probably means API changes.
Comment 17 Jasper St. Pierre (not reading bugmail) 2016-02-19 18:24:00 UTC
(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?
Comment 18 Carlos Garnacho 2016-02-20 19:06:33 UTC
(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).
Comment 19 Jasper St. Pierre (not reading bugmail) 2016-02-20 19:08:57 UTC
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
Comment 20 Carlos Garnacho 2016-02-20 20:56:55 UTC
gtk_window_set_startup_id() is called nowhere by gtk+ :)
Comment 21 Giovanni Campagna 2016-02-20 21:58:49 UTC
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.