Created attachment 158979 [details] [review]
[ShellApp] Add methods to focus an app

The design calls for raising all windows for a given app in
certain circumstances; implement this.

Created attachment 158980 [details] [review]
[altTab] Raise all windows for an app

Comment on attachment 158979 [details] [review]
[ShellApp] Add methods to focus an app

>+          if (other_window != window)
>+            meta_window_raise_with_transients (other_window);

ah... so the issue with the mutter patch is perhaps that meta_window_activate() handles raising transients, but meta_window_raise() doesn't?

Comment on attachment 158980 [details] [review]
[altTab] Raise all windows for an app

looks good, modulo issues with other patches

*** Bug 615195 has been marked as a duplicate of this bug. ***

Created attachment 159017 [details] [review]
[ShellApp] Add methods to focus an app

The design calls for raising all windows for a given app in
certain circumstances; implement this.

Created attachment 159489 [details] [review]
Add methods to focus an app

The design calls for raising all windows for a given app in
certain circumstances; implement this.

Review of attachment 159489 [details] [review]:

Looks good except for minor comments.

::: src/shell-app.c
@@ +249,2 @@
 /**
+ * shell_app_focus:

Let's call this 'activate' not 'focus'

@@ +288,3 @@
+static MetaWindow *
+find_most_recent_transient_on_same_workspace (MetaDisplay *display,
+                                              MetaWindow  *source)

What's "source" about this? Maybe chosen_window or something?

@@ +297,3 @@
+
+  transients = NULL;
+  meta_window_foreach_transient (source, collect_transients, &transients);

I'd say use a small closure structure to pass in the source workspace along with the GList *, and then make collect_transients check the workspace, rather than doing a subsequent messy filtering step.

(Or use g_slist_filter, but no reason to write two callbacks)

@@ +302,3 @@
+    {
+      MetaWindow *transient = iter->data;
+      MetaWorkspace *transient_workspace = meta_window_get_workspace (transient);

Thnk you should restrict this to transients with type {NORMAL,DIALOG} - if you have a UTILITY or TOOLBAR transient, say, then it shouldn't get keyboard focused. (I thought this would break in the GIMP, but the GIMP's tool windows are transient for the entire group and not the image window.)

@@ +321,3 @@
+  iter = g_slist_last (transients_sorted);
+  if (iter)
+    result = iter->data;

Shorter and I clearer as:

if (transients_sorted != NULL)
   result = g_slist_last (transients_sorted)->data;
else
   result = NULL;

Review of attachment 158980 [details] [review]:

This doesn't seem right to me to restrict this to Alt-Tab - we definitely want the same behavior for clicking on apps in the dash.

I'm less sure about cases where there is no ambiguity in which window you meant to go to:

 - selecting a window in the overview - probably? - if the windows are related, you want to interact with all of them. If the windows are two Firefox main windows, then you probably didn't want to interact with both and could be annoying if you are trying to set up a two-window workflow.

 - Clicking on a window - probably not? - if someone wants to carefully restack their windows, we probably don't want to enforce them acting as a layer. (Though if we could handle the two Firefox main windows situation better, maybe it's OK to handle other apps which are more like a pile of related windows as a layer.)

Maybe just do the dash-activation for now?

(In reply to comment #8)
> Review of attachment 159489 [details] [review]:
> 
> Looks good except for minor comments.
> 
> ::: src/shell-app.c
> @@ +249,2 @@
>  /**
> + * shell_app_focus:
> 
> Let's call this 'activate' not 'focus'

There's already a method called activate (which is focus-if-running-else-launch).  I'm not feeling an immediate inspiration for different names here.

> 
> @@ +288,3 @@
> +static MetaWindow *
> +find_most_recent_transient_on_same_workspace (MetaDisplay *display,
> +                                              MetaWindow  *source)
> 
> What's "source" about this? Maybe chosen_window or something?

I picked "reference".

> 
> @@ +297,3 @@
> +
> +  transients = NULL;
> +  meta_window_foreach_transient (source, collect_transients, &transients);
> 
> I'd say use a small closure structure to pass in the source workspace along
> with the GList *, and then make collect_transients check the workspace, rather
> than doing a subsequent messy filtering step.

Done.

> Thnk you should restrict this to transients with type {NORMAL,DIALOG} - if you
> have a UTILITY or TOOLBAR transient, say, then it shouldn't get keyboard
> focused. (I thought this would break in the GIMP, but the GIMP's tool windows
> are transient for the entire group and not the image window.)

Done.

> Shorter and I clearer as:
> 
> if (transients_sorted != NULL)
>    result = g_slist_last (transients_sorted)->data;
> else
>    result = NULL;

This code needed to be reworked for the above anyways, so fixed.

(In reply to comment #9)
> Review of attachment 158980 [details] [review]:
> 
> This doesn't seem right to me to restrict this to Alt-Tab - we definitely want
> the same behavior for clicking on apps in the dash.

This was probably unclear, but we get that behavior from the first patch because it changes _activate() (which is what the app well uses) to call _focus.

>  - selecting a window in the overview - probably? - if the windows are related,
> you want to interact with all of them. If the windows are two Firefox main
> windows, then you probably didn't want to interact with both and could be
> annoying if you are trying to set up a two-window workflow.

Yeah, we'll need to think about this.  I can't immediately find a section of the design doc that talks about this.  Jon?
 
>  - Clicking on a window - probably not? - if someone wants to carefully restack
> their windows, we probably don't want to enforce them acting as a layer.
> (Though if we could handle the two Firefox main windows situation better, maybe
> it's OK to handle other apps which are more like a pile of related windows as a
> layer.)

Right.  Though I'm tending to think here that anyone who wants careful window stacking is going to be fighting the system and ultimately be frustrated; we will need to look at the workflow of someone who does this now and see how we can handle it.

Attachment 158980 [details] pushed as 35bf6b0 - [altTab] Raise all windows for an app