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 616051 - Make alt-tab more app-based
Make alt-tab more app-based
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
: 615195 (view as bug list)
Depends on: 616050
Blocks:
 
 
Reported: 2010-04-17 20:58 UTC by Colin Walters
Modified: 2010-05-06 15:20 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[ShellApp] Add methods to focus an app (5.23 KB, patch)
2010-04-17 20:58 UTC, Colin Walters
reviewed Details | Review
[altTab] Raise all windows for an app (1.88 KB, patch)
2010-04-17 20:58 UTC, Colin Walters
committed Details | Review
[ShellApp] Add methods to focus an app (5.23 KB, patch)
2010-04-18 15:26 UTC, Colin Walters
none Details | Review
Add methods to focus an app (6.92 KB, patch)
2010-04-24 21:22 UTC, Colin Walters
committed Details | Review

Description Colin Walters 2010-04-17 20:58:30 UTC
See patches
Comment 1 Colin Walters 2010-04-17 20:58:32 UTC
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.
Comment 2 Colin Walters 2010-04-17 20:58:35 UTC
Created attachment 158980 [details] [review]
[altTab] Raise all windows for an app
Comment 3 Dan Winship 2010-04-17 22:51:23 UTC
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 4 Dan Winship 2010-04-17 22:55:08 UTC
Comment on attachment 158980 [details] [review]
[altTab] Raise all windows for an app

looks good, modulo issues with other patches
Comment 5 drago01 2010-04-18 11:57:54 UTC
*** Bug 615195 has been marked as a duplicate of this bug. ***
Comment 6 Colin Walters 2010-04-18 15:26:28 UTC
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.
Comment 7 Colin Walters 2010-04-24 21:22:55 UTC
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.
Comment 8 Owen Taylor 2010-05-05 18:31:43 UTC
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;
Comment 9 Owen Taylor 2010-05-05 18:41:44 UTC
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?
Comment 10 Colin Walters 2010-05-06 14:27:07 UTC
(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.
Comment 11 Colin Walters 2010-05-06 14:54:34 UTC
(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.
Comment 12 Colin Walters 2010-05-06 15:20:03 UTC
Attachment 158980 [details] pushed as 35bf6b0 - [altTab] Raise all windows for an app