GNOME Bugzilla – Bug 752677
x11: implement fullscreen_on_monitor
Last modified: 2018-04-15 00:25:58 UTC
Created attachment 307843 [details] [review] Added api to set a GtkWindow fullscreen on a given monitor. When setting an application window to fullscreen and multiple monitors are present there is no api to tell which monitor to go fullscreen onto. Attached is a patch which adds this api and implements it in the Wayland backend. Once it's been reviewed I'll implement the same in the X11 backend also.
Is there a use case for fullscreening on another monitor than the one you're on ?
I think the use-case is really wayland because we can't move the window to a specific position. On X you would take the monitor's geometry, move the window there, then fullscreen. On normal desktop usage (i.e. GNOME) I guess it makes little sense to control where to fullscreen, but our app (I'm working with Jeremy) is made for a specific setup where we've 2 monitors and we create 2 fullscreen windows, one per monitor. Without this API, on wayland, we can only fullscreen both on the first monitor, we can't place anything on the 2nd.
Another use case: a presentation app that will put the slide deck in presentation mode on another, non-primary display and then full screen it.
(In reply to Emmanuele Bassi (:ebassi) from comment #3) > Another use case: a presentation app that will put the slide deck in > presentation mode on another, non-primary display and then full screen it. I think that is more like bug 750280 ? But point taken
(In reply to Xavier Claessens from comment #2) > I think the use-case is really wayland because we can't move the window to a > specific position. On X you would take the monitor's geometry, move the > window there, then fullscreen. i.e. specifically fullscreening a window on another output. Why bother moving it, having it displayed there, then requesting fullscreen, when you could just do the entire thing in one atomic request, which also means that 'unfullscreening' is easy since your window moves back to where it was, rather than to its intermediate position?
@Daniel: true. Was just pointing that in X world there is a workaround that I suspect some applications does. In wayland new API is mandatory.
Review of attachment 307843 [details] [review]: ::: gdk/gdkwindow.h @@ +947,3 @@ GDK_AVAILABLE_IN_ALL void gdk_window_fullscreen (GdkWindow *window); +GDK_AVAILABLE_IN_3_16 18, at this point ::: gdk/wayland/gdkscreen-wayland.c @@ +1133,3 @@ +{ + GdkWaylandScreen *screen_wayland = GDK_WAYLAND_SCREEN (screen); + GdkWaylandMonitor *monitor = g_ptr_array_index (screen_wayland->monitors, monitor_num); Using g_ptr_array_index here seems risky - thats a direct array access, without bounds checking. ::: gdk/wayland/gdkwindow-wayland.c @@ +131,3 @@ int margin_bottom; + + struct wl_output *fullscreen_output; Why do you remember the output here ? It doesn't seem to be used for anything. ::: gtk/gtkwindow.c @@ +10129,3 @@ + * + * Since: UNRELEASED + */ 3.18 here @@ +10131,3 @@ + */ +void +gtk_window_fullscreen_on_monitor(GtkWindow *window, GdkScreen *screen, gint monitor) space before ( and please line up the parameters @@ +10153,3 @@ + + if (toplevel != NULL) + gdk_window_fullscreen_on_monitor (toplevel, monitor); This code has recently changed, you'll have to rebase your patch ::: gtk/gtkwindow.h @@ +387,3 @@ void gtk_window_unfullscreen (GtkWindow *window); +GDK_AVAILABLE_IN_3_16 +void gtk_window_fullscreen_on_monitor(GtkWindow *window, GdkScreen *screen, gint monitor); 18, again. Also please line up the arguments, and add a space before (
(In reply to Matthias Clasen from comment #7) > ::: gdk/wayland/gdkscreen-wayland.c > @@ +1133,3 @@ > +{ > + GdkWaylandScreen *screen_wayland = GDK_WAYLAND_SCREEN (screen); > + GdkWaylandMonitor *monitor = g_ptr_array_index (screen_wayland->monitors, > monitor_num); > > Using g_ptr_array_index here seems risky - thats a direct array access, > without bounds checking. All other functions in that file assume that base class checked it already. GdkScreen does it: g_return_val_if_fail (monitor_num < gdk_screen_get_n_monitors (screen), -1); > ::: gdk/wayland/gdkwindow-wayland.c > @@ +131,3 @@ > int margin_bottom; > + > + struct wl_output *fullscreen_output; > > Why do you remember the output here ? It doesn't seem to be used for > anything. It is used to remember on which monitor to fullscreen on in the case the xdg_surface wasn't created yet.
(In reply to Xavier Claessens from comment #8) > (In reply to Matthias Clasen from comment #7) > > ::: gdk/wayland/gdkscreen-wayland.c > > @@ +1133,3 @@ > > +{ > > + GdkWaylandScreen *screen_wayland = GDK_WAYLAND_SCREEN (screen); > > + GdkWaylandMonitor *monitor = g_ptr_array_index (screen_wayland->monitors, > > monitor_num); > > > > Using g_ptr_array_index here seems risky - thats a direct array access, > > without bounds checking. > > All other functions in that file assume that base class checked it already. > GdkScreen does it: g_return_val_if_fail (monitor_num < > gdk_screen_get_n_monitors (screen), -1); Ah, right. > > > ::: gdk/wayland/gdkwindow-wayland.c > > @@ +131,3 @@ > > int margin_bottom; > > + > > + struct wl_output *fullscreen_output; > > > > Why do you remember the output here ? It doesn't seem to be used for > > anything. > > It is used to remember on which monitor to fullscreen on in the case the > xdg_surface wasn't created yet. Oh right. But then it seems racy: 1) call fullscreen_on_monitor on an unmapped window 2) unplug that monitor 3) show the window 4) access to an already-freed GdkWaylandMonitor struct
Created attachment 308032 [details] [review] Updated with Matthias Clasen's suggestions and rebased on master.
Created attachment 308033 [details] [review] Updated to only set fullscreen_initially if there's no toplevel widget.
Matthias, I can see that race condition, how would we work around that though? Is there some signal sent when a monitor is unplugged? I guess we could null the monitor when we get that signal or something.
(In reply to Matthias Clasen from comment #9) > Oh right. But then it seems racy: > > 1) call fullscreen_on_monitor on an unmapped window > 2) unplug that monitor > 3) show the window > 4) access to an already-freed GdkWaylandMonitor struct Hm, good point! That race can occur in 2 places actually: - In GtkWindow monitors can change while we are waiting for gtk_window_map() to set the initial state. That won't crash but hit a g_return_if_fail() in gdk_window_fullscreen_on_monitor() called from gtk_window_map(). - In GdkWindowImplWayland monitors can change while we are waiting for the xdg_surface. In that case we keep the fullscreen_output pointer to a GdkWaylandMonitor object that could be destroyed if we use it later in gdk_wayland_window_create_xdg_surface(). So the problem is not the g_ptr_array_index() call AFAIK. In both cases we should reset the value when GdkScreen emits "monitors-changed" signal.
Created attachment 308037 [details] [review] Updated to only store monitor number and check it before using it.
Review of attachment 308037 [details] [review]: ::: gdk/wayland/gdkwindow-wayland.c @@ +1014,3 @@ if (window->state & GDK_WINDOW_STATE_MAXIMIZED) xdg_surface_set_maximized (impl->xdg_surface); + if (window->state & GDK_WINDOW_STATE_FULLSCREEN && fullscreen_output != NULL) You shouldn't check for fullscreen_output != NULL here, it breaks the case where you don't specify on which monitor to fullscreen. @@ +1948,3 @@ return; + impl->initial_fullscreen_monitor = 0; the "unset" value is -1 not 0 ::: gtk/gtkwindow.c @@ +10285,3 @@ + priv->fullscreen_initially = FALSE; + priv->initial_fullscreen_monitor = -1; Hmm, I think you're actually right here, that's a regression introduced by Matthias in commit f30637bbae4686ed78dd0976cb941ebab9e28cca, in this case we want to unconditionally reset the initial value. But you should remove the "else" case at the end of that function then. If I understand correctly, here is what could happen (upstream bug, not your fault): 1) When window is not yet mapped, do gtk_window_fullscreen(), fullscreen_initially is set to TRUE. 2) Window gets mapped, it gets fullscreened 3) Call gtk_window_unfullscreen() and fullscreen_initially is NOT reset to FALSE (in current gtk master) 4) unmap and re-map the window => window goes fullscreen again.
Maybe another way to fix it is to reset all those _initially variables to FALSE at the end of gtk_window_map(). That would respect more Matthias' intention "Go back to use these variables only for pre-mapped state changes." In that case if you do the sequence fullscreen, map, unmap, map the result is a unfullscreened window which is fair because unmap "loose" the state of the window. It's also consistent with the sequence: map, fullscreen, unmap, map which would also result in unfullscreened window.
I'm afraid the patch will need one more round of rebasing, since I fixed that regression that Xavier mentioned, last night.
(In reply to Xavier Claessens from comment #15) > If I understand correctly, here is what could happen (upstream bug, not your > fault): > 1) When window is not yet mapped, do gtk_window_fullscreen(), > fullscreen_initially is set to TRUE. > 2) Window gets mapped, it gets fullscreened > 3) Call gtk_window_unfullscreen() and fullscreen_initially is NOT reset to > FALSE (in current gtk master) > 4) unmap and re-map the window => window goes fullscreen again. This is not happening, btw - we record the current state in unmap, fullscreen_initially _will_ be reset there.
Created attachment 308104 [details] [review] Updated after master changed.
Review of attachment 308104 [details] [review]: This looks pretty good to me now, There's still some minor coding style things, but I've already forced you through multiple rounds by pushing changes to the same code, so lets call this good enough.
Ok, merged, I think I'll leave this bug open to remind me to add an X11 implementation next week.
Any update on that x11 implementation ?
Wow, over a month later, sorry. I'll look at the X11 backend this week.
Created attachment 311194 [details] [review] Patch to implement the new api on X11 backend This new patch feels incomplete. I'm not sure where to check for the initial_fullscreen_monitor and react accordingly like the wayland backend had in gdk_wayland_window_create_xdg_surface. Any pointers are welcome and I'll improve the patch until it does what it is supposed to.
Created attachment 330257 [details] Reproducer Hi, I've tried the new function but it doesn't work for me on Wayland. Attached is a reproducer which should open fullscreen windows on all monitors for 3 seconds (I've tested it on Fedora 24 with Gnome on Wayland).
We're moving to gitlab! As part of this move, we are moving bugs to NEEDINFO if they haven't seen activity in more than a year. If this issue is still important to you and still relevant with GTK+ 3.22 or master, please reopen it and we will migrate it to gitlab.
As announced a while ago, we are migrating to gitlab, and bugs that haven't seen activity in the last year or so will be not be migrated, but closed out in bugzilla. If this bug is still relevant to you, you can open a new issue describing the symptoms and how to reproduce it with gtk 3.22.x or master in gitlab: https://gitlab.gnome.org/GNOME/gtk/issues/new