Created attachment 158976 [details] [review]
[MetaDisplay] Export methods interact with user_time

This is useful when calling some of the lower level mutter functions,
e.g. controlling window stacking.

Created attachment 158977 [details] [review]
[MetaWindow] Export the functions to control demands_attention

Plugins can want a finer grained control over this.

Created attachment 158978 [details] [review]
Add functionality for raising/activating a window, respecting transients

In an application-based system like GNOME Shell, we often transition
between applications, and not particular windows.  This patch adds
utility functions which allow raising a toplevel window, but
respecting its transients.

This fixes e.g. opening a file chooser, alt-tab'ing away, then tabbing
back.

Comment on attachment 158977 [details] [review]
[MetaWindow] Export the functions to control demands_attention

ok, but would it make more sense as a boolean gobject property?

Comment on attachment 158978 [details] [review]
Add functionality for raising/activating a window, respecting transients

>In an application-based system like GNOME Shell, we often transition
>between applications, and not particular windows.  This patch adds
>utility functions which allow raising a toplevel window, but
>respecting its transients.
>
>This fixes e.g. opening a file chooser, alt-tab'ing away, then tabbing
>back.

For me, with unpatched mutter, the file chooser is already kept above the main window, and raised along with it when the window is activated. The only problem is that it doesn't get keyboard focus when I Alt-Tab back to it (the main window does instead, although it remains stacked underneath the file chooser). But IMHO that's just a bug, and should be fixed in ordinary meta_window_activate().

Is that not the behavior you're seeing?

>+ * Returns: (transfer container): List of transients for this window, sorted by user time

Should say "sorted in stacking order, bottom to top", right?

Comment on attachment 158976 [details] [review]
[MetaDisplay] Export methods interact with user_time

this does what it says it does, though i wonder why this function hasn't been needed up to this point. is the existing code doing the comparison incorrectly? or is everything that has to compare timestamps in display.c, even if it doesn't logically belong there?

(In reply to comment #5)
> 
> 
> For me, with unpatched mutter, the file chooser is already kept above the main
> window, and raised along with it when the window is activated. The only problem
> is that it doesn't get keyboard focus when I Alt-Tab back to it (the main
> window does instead, although it remains stacked underneath the file chooser).
> But IMHO that's just a bug, and should be fixed in ordinary
> meta_window_activate().

Hmm, yes maybe you're right it's just a bug in meta_window_activate.  I sort of went to an elaborate workaround to not change that, but if we agree that's right then let's definitely fix it there.

> 
> Is that not the behavior you're seeing?
> 
> >+ * Returns: (transfer container): List of transients for this window, sorted by user time
> 
> Should say "sorted in stacking order, bottom to top", right?

Yes

(In reply to comment #6)
> (From update of attachment 158976 [details] [review])
> this does what it says it does, though i wonder why this function hasn't been
> needed up to this point. is the existing code doing the comparison incorrectly?
> or is everything that has to compare timestamps in display.c, even if it
> doesn't logically belong there?

It's to export it basically; it lived in display-private.h, now we can use it from both plugins and script.

Created attachment 159016 [details] [review]
meta_window_activate should respect transients

When e.g. using Alt-TAB to move between windows, we should by
default respect transients for the given window.

This fixes e.g. opening a file chooser in GEdit, alt-tab'ing
away, then tabbing back.

--- a/src/core/window.c
+++ b/src/core/window.c
@@ -3526,6 +3527,7 @@ window_activate (MetaWindow     *window,
   if (window->xtransient_for == None &&
       !meta_window_located_on_workspace (window, workspace))
     {
+      meta_window_set_user_time (window, timestamp);
       meta_window_set_demands_attention (window);
       /* We've marked it as demanding, don't need to do anything else. */
       return;

This is not right; the usertime should be the last time the user directly interacted with the window (e.g., typed into it or clicked on it). Since this window is not getting actually activated, we can't change the user time.

@@ -3534,22 +3536,51 @@ window_activate (MetaWindow     *window,
     {
       /* Move transients to current workspace - preference dialogs should appear over
          the source window.  */
-    meta_window_change_workspace (window, workspace);
+      meta_window_change_workspace (window, workspace);
+      meta_window_raise (window);

I think this will break things if the 'raise on click' preference is not set.

+      if (transients)
+        {
+          target = g_slist_last (transients)->data;

You seem to be assuming that if the window has transients, the focused window is one of the transients, and this assumption cannot be granted (e.g., a common reason for non-modal dialogs is to allow the user to interact with the main application while the dialog is visible above it).
 
+  for (iter = transients; iter; iter = iter->next)
+    {
+      MetaWindow *window = iter->data;
+      meta_window_raise (window);
+    }

Is the assumption that the usertime sequence represents the correct stacking order (e.g., as created by the application & user) valid ? If it is, then the transients should already be in that order so this would be superfluous. If this is not superfluous, the assumption is probably not correct.

IMO restacking all transients in window_activate() is not the correct thing to do; the window_activate() function should do what the name implies, activate the window it was given. Anything else will break things for applications that use non-modal dialogs in a sensible way and for external pagers.

I think the problem is basically with the paging algorithm rather than the actual activation (note this only happens when the dialog is not modal to the application; e.g., the GEdit FileSave dialog does not exhibit this problem, while the FileOpen does). Inside the pager, determining which window to activate based on the usertime might indeed be the right/best thing to do, though this needs to include not just the transients, but also the application window usertime to avoid stealing focus from the main window.

So, I think we need something like the new API proposed in comment 3, but the implementation should consider the usertime of the application window as well, so that the focus goes not go to the topmost window, but the one the user interacted with last.

Created attachment 159486 [details] [review]
Export meta_window_raise and meta_window_lower

For plugins that want fine grained control over window stacking.

Created attachment 159487 [details] [review]
Add public function to sort windows by stacking

Created attachment 159488 [details] [review]
Export functions to iterate over window transients

Note on these patches - rather than trying to handle this in meta_window_activate which is also used in other cases like click handling, I chose to keep mutter as a lower level library, and implement the handling inside the shell plugin.

Review of attachment 159487 [details] [review]:

I'd like to see docs - see what you think this function and what it's good for before I dig into the stacking code again and try to figure out if that's what it actually does.

Review of attachment 159486 [details] [review]:

Looks OK.

Review of attachment 159488 [details] [review]:

Need to add function docs, these functions (especially foreach_ancestor) aren't obvious in what they do.

Review of attachment 159487 [details] [review]:

What I'm using this for is to sort the transient windows for a given window, so we can call meta_window_raise on them in the right order.

Created attachment 159710 [details] [review]
Export functions to iterate over window transients

(In reply to comment #18)
> Review of attachment 159487 [details] [review]:
> 
> What I'm using this for is to sort the transient windows for a given window, so
> we can call meta_window_raise on them in the right order.

Can you write that down a doc comment, with that as an example use case and with a bit more detail? One important detail is that if override-redirect windows are included in the list of windows, the returned stacking order will be undefined.

Created attachment 159837 [details] [review]
Add public function to sort windows by stacking

Review of attachment 159837 [details] [review]:

Rewrite of the doc comment below. I think it's important to specify the operation of the function fairly carefully. OK to commit with my rewrite.

::: src/core/display.c
@@ +5020,3 @@
+ * windows for another window, or to sort a set of toplevel windows.
+ * The ordering of this function is not defined if any override-redirect
+ * windows are in @windows.

Sorts a set of windows according to their current stacking order. If windows from multiple screens are present in the set of input windows, then all the windows on screen 0 are sorted below all the windows on screen 1, and so forth. Since the stacking order of override-redirect windows isn't controlled by Metacity, if override-redirect windows are in the input, the result may not correspond to the actual stacking order in the X server.

An example of using this would be to sort the list of transient dialogs for a window into their current stacking order.

Review of attachment 159710 [details] [review]:

OK to commit with two tiny fixes

::: src/core/window.c
@@ +8253,3 @@
+ * @window: a #MetaWindow
+ * @func: (scope call): Called for each window which is a transient of @window (transitively)
+ * @data: (closure): User data

You renamed this to @user_data

@@ +8292,3 @@
+ * @window: a #MetaWindow
+ * @func: (scope call): Called for each window which is a transient parent of @window
+ * @data: (closure): User data

And this

Attachment 159486 [details] pushed as 4994087 - Export meta_window_raise and meta_window_lower
Attachment 159710 [details] pushed as 609aae6 - Export functions to iterate over window transients
Attachment 159837 [details] pushed as fd20059 - Add public function to sort windows by stacking