GNOME Bugzilla – Bug 591763
appWell: Highlight windows related to application on mouseover
Last modified: 2009-09-08 19:58:30 UTC
Currently implemented by fading all the other windows. We probably want a better visual effect here, but posting this patch as a work in progress.
Created attachment 140736 [details] [review] appWell: Highlight windows related to application on mouseover
Comment on attachment 140736 [details] [review] appWell: Highlight windows related to application on mouseover The eye naturally goes to the windows that change (go dark) and not to the windows that don't change. Design that I think would work a bit etter better: - As soon as the mouse enters the well, quickly fade a slightly dark overlay over the whole workspaces area. (Not nearly as dark as what you have now - maybe 0x00000044 rather than the 192*0x000000ff == 0x000000cc) - Raise just the highlighted window(s) above that as you mouse over the icons. (Possibly as a fade, though not sure it's worth the code to implement - we don't fade on alt-Tab) But it is also inherently perhaps distracting to have stuff changing on the right as you mouse over the favorites well. Needs design refinement.
you could have the highlighted windows grow a bit (reusing code from the scroll-wheel-zooming patch)
This should probably be a click and hold effect. How about the following? * On click and hold, smoothly zoom into the windows for that app - showing only those windows. And also show the popup menu of window titles next to the app icon. * As I move over the window titles in the list zoom a bit more into that window * If I initially release over an item in the popup, list go directly to that window * If I initially release over the app icon itself, keep the window list popped up and the windows zoomed in. I can then click on an item in the list, a zoomed window, or somewhere else to popdown the menu and resume the normal mode. * If I initially release outside the list or app icon, popdown the menu. * When the menu pops down (goes away) smoothly zoom out of the visible windows. They should return to the previous location.
Click and hold feels like it might conflict a bit with drag and drop. Maybe we only start drag and drop when you move the mouse completely outside? One other thing related to this - should we show the full application name in some way on click and hold? Or is that just mouseover? Otherwise this all sounds good, I'll see about implementing.
Well after thinking about it, I am not sure that mixing the apps/windows from multiple workspaces is a good idea, for the sole reason that it kinda defeats the purpose of workspaces. In metacity we only should the windows of the current workspace by default in the tasklist. The reason for using workspaces is to be able to somehow group the windows to limit the window selector to the ones you care about. Example: I have xchat, 5 firefox windows and thunderbird open on workspace 1. On workspace 2 have a bunch of terminals, gedit and two firefox windows. Now when I am working on workspace 2 and want to change to firefox I don't really want to so the ones in workspace 1 at all, If I wanted that than why did I open them in a different workspace to begin with. Now the problem is that there isn't really an active workspace in the overlay mode, but assuming the workspace that was active before opening the overlay is the one the user is focusing on might make sense (unless he opened the overlay to change the workspace). But when changing workspace I doubt that someone would use the app wheel for this but either clicking on the workspace (or a window in it), or drag an app/document onto it. So my suggestion is to limit the app wheel to the current workspace. Thoughts?
There is an active workspace in overview, it has a white border. Admittedly this isn't very visible. If we sort the window list into two parts, ones on the current workspace, then a ---- separator, then windows on other workspaces would that help your case?
(In reply to comment #7) > There is an active workspace in overview, it has a white border. Admittedly > this isn't very visible. > > If we sort the window list into two parts, ones on the current workspace, then > a ---- separator, then windows on other workspaces would that help your case? Yes its at least way better than just mixing them.
Colin, probably don't want the full app name to be shown when the menu is up.
Created attachment 141703 [details] [review] Split out "AppIcon" and make WellDisplayItem a subclass of it In hopes of avoiding further divergence, here's the work I did to make the well icon code shareable with the Alt-Tab dialog. (Per the spec, the Alt-Tab dialog will want the drop-down window menu, but NOT the drag-and- drop-ability. But it may be necessary to implement dnd in AppIcon rather than WellDisplayItem anyway, just to get the grabs and stuff right
Comment on attachment 141703 [details] [review] Split out "AppIcon" and make WellDisplayItem a subclass of it >@@ -552,7 +493,7 @@ WellDisplayItem.prototype = { > }, > > getDragActor: function(stageX, stageY) { >- return this.appInfo.create_icon_texture(APP_ICON_SIZE); >+ return new Clutter.Clone({ source: this._icon }); > }, > > // Returns the original icon that is being used as a source for the cloned texture Can we do that? I thought there were bugs in using clones as drag and drop sources. (But I don't recall the bug if any). Also I'd slightly prefer to see it remain inside appDisplay.js instead of being a new toplevel file since it feels cleaner, but it's not a strong preference.
(In reply to comment #11) > >- return this.appInfo.create_icon_texture(APP_ICON_SIZE); > >+ return new Clutter.Clone({ source: this._icon }); > Can we do that? I thought there were bugs in using clones as drag and drop > sources. (But I don't recall the bug if any). I did a really basic test (dragged it to the favorites area) and it seemed to work fine, but there might be edge cases that fail... > Also I'd slightly prefer to see it remain inside appDisplay.js instead of being > a new toplevel file since it feels cleaner, but it's not a strong preference. Well, except AppItem will be getting used by altTab.js too, so it seemed to make sense to have it be off on its own, rather than having altTab reaching into appDisplay and pulling out a random type. But if you'd rather keep it in appDisplay we can. (If you do, you can get rid of the getDragActor change too if you want, since the point of that was that APP_ICON_SIZE was now a variable in appIcon.js and so appDisplay.js didn't have access to it any more.)
Comment on attachment 141703 [details] [review] Split out "AppIcon" and make WellDisplayItem a subclass of it committed with change to not use a clone for dnd
Created attachment 141819 [details] [review] Fix allocation implementations for ShellStack and ShellDrawingArea In both, using our allocation directly for the child is wrong; we should create a new allocation that's our width and height. In ShellDrawingArea, also need to chain up to parent.
Created attachment 141820 [details] [review] New class ShellButtonBox This is a Box subclass which adds several signals useful for implementing "button like" behavior, such as hover and pressed states, as well as click activation on release.
Created attachment 141821 [details] [review] Display a window menu in the application well for multiple windows Split the well item into two different classes depending on running versus not. The non-running case uses the ShellButtonBox for button-like behavior. The running case grabs the pointer and implements a menu directly. We should probably have better handling of modal popups in the shell though; e.g. we likely want to grab the keyboard too to handle Escape. The mouseover animation is just a placeholder until the zooming patch lands. Log statements still left in.
Comment on attachment 141821 [details] [review] Display a window menu in the application well for multiple windows Work in progress, but useful to play with for a feel. I personally don't really ever have multiple windows for any app, so it's not that useful to me (well, if I do they're transients which we should handling saner). And except for the Gimp, but that one cries out for the application based window handling. Playing with it a bit though opening multiple terminal windows and firefox windows, it seems OK; we should probably axe the window icon unless it's different from the application icon. The other annoying thing is long titles obscuring the windows. Well, and the fact that the window title often has the application name in it. Without implementing it though, I think i'd prefer if we didn't pop up a menu, but instead just hid all the other windows, and did a window title popup right above the window.
(In reply to comment #17) > Without implementing it though, I think i'd prefer if we didn't pop up a menu, > but instead just hid all the other windows, and did a window title popup right > above the window. Well but this would slow down the workflow when switching windows, you'd had to click the icon and search for the window, move the mouse to it and click. Rather than click the icon, select the window from the menu, done.
(In reply to comment #18) > (In reply to comment #17) > > > Without implementing it though, I think i'd prefer if we didn't pop up a menu, > > but instead just hid all the other windows, and did a window title popup right > > above the window. > > Well but this would slow down the workflow when switching windows, you'd had to > click the icon and search for the window, move the mouse to it and click. > > Rather than click the icon, select the window from the menu, done. Hm, I guess it depends how many windows the app has. If you have just say 2-3, then the windows will be large and easy click targets; in the browser case possibly even recognizable. If you have say 9 though (and any other windows), then hiding other windows won't make them proportionally much bigger. Wait...Jon's comment says to do this on hold+hover and I missed it. I'll update the patch.
(In reply to comment #19) > (In reply to comment #18) > > (In reply to comment #17) > > > > > Without implementing it though, I think i'd prefer if we didn't pop up a menu, > > > but instead just hid all the other windows, and did a window title popup right > > > above the window. > > > > Well but this would slow down the workflow when switching windows, you'd had to > > click the icon and search for the window, move the mouse to it and click. > > > > Rather than click the icon, select the window from the menu, done. > > Hm, I guess it depends how many windows the app has. If you have just say 2-3, > then the windows will be large and easy click targets; in the browser case > possibly even recognizable. If you have say 9 though (and any other windows), > then hiding other windows won't make them proportionally much bigger. Yeah that is exactly my point, if you have a few windows open id does not matter much, but in this case you can just click on them in the overlay anyway. Also note that the windows are smaller when multiple workspaces are in use.
Comment on attachment 141819 [details] [review] Fix allocation implementations for ShellStack and ShellDrawingArea >+ /* chain up to set actor->allocation */ >+ (CLUTTER_ACTOR_CLASS (g_type_class_peek (clutter_actor_get_type ())))->allocate (self, box, flags); CLUTTER_ACTOR_CLASS (shell_drawing_area_parent_class)->allocate (self, box, flags); (Likewise with the existing chain-up in shell-stack.c.) (You don't need to define shell_drawing_area_parent_class yourself; G_DEFINE_TYPE does.)
Comment on attachment 141820 [details] [review] New class ShellButtonBox > shell-arrow.h \ >+ shell-button-box.c \ >+ shell-button-box.h \ > shell-drawing.c \ tabs/spaces >+static gboolean >+shell_button_box_on_enter (ShellButtonBox *box, >+static gboolean >+shell_button_box_on_leave (ShellButtonBox *box, >+static gboolean >+shell_button_box_on_press (ShellButtonBox *box, >+static gboolean >+shell_button_box_on_release (ShellButtonBox *box, Why are these needed if you're connecting to captured-event too? If they are needed, shouldn't you be clearing "hover" in on_leave? >+ /** >+ * ShellButtonBox::hover I think gtk-doc expects two colons for signal name definitions, but only one for property names. (At least, gtk consistently does it that way, and I assume this is not just a historical accident. :-) >+ * This property tracks whether the mouse is hovering over the button. whether or not the mouse button is pressed, right? Would be good to add that explicitly.
Comment on attachment 141821 [details] [review] Display a window menu in the application well for multiple windows quick semi-review since you're redoing this anyway... >+function WellMenu(source) { Doh... I thought you realized this; the menu is needed by the Alt-Tab dialog as well, so this should be part of AppIcon, not part of WellDisplayItem. The selected-window-highlighting part would work differently in Alt-Tab though, so that would have to be a signal emitted by AppIcon or something, that the overview and app switcher would implement differently.
(In reply to comment #21) > (From update of attachment 141819 [details] [review]) > >+ /* chain up to set actor->allocation */ > >+ (CLUTTER_ACTOR_CLASS (g_type_class_peek (clutter_actor_get_type ())))->allocate (self, box, flags); > > CLUTTER_ACTOR_CLASS (shell_drawing_area_parent_class)->allocate (self, box, > flags); > > (Likewise with the existing chain-up in shell-stack.c.) > (You don't need to define shell_drawing_area_parent_class yourself; > G_DEFINE_TYPE does.) This is kind of a hack, I don't actually want to use the drawing area's parent, because that's ClutterGroup, which will allocate the children. I'm trying to override that. So we call directly into the ClutterActor class's allocate.
Created attachment 141898 [details] mockups of menus (from Jeremy) I sent this directly into Colin's gmail spam folder a while ago :)
Comment on attachment 141820 [details] [review] New class ShellButtonBox Pushed with fixes for review comments as well as other changes; mainly it uses a pointer grab now.
Created attachment 141972 [details] [review] Display a window menu in the application well for multiple windows Split the well item into two different classes depending on running versus not. The non-running case uses the ShellButtonBox for button-like behavior. The running case grabs the pointer and implements a menu directly. We should probably have better handling of modal popups in the shell though; e.g. we likely want to grab the keyboard too to handle Escape. The mouseover animation is just a placeholder until the zooming patch lands. Log statements still left in.
Still todo with above patch: * Remove window icons if same as app * Workspaces differentiation * Rework visually to look like latest mockup UI concerns: * The menu easily overlaps the first workspace entirely with just 4 workspaces.
(In reply to comment #25) > Created an attachment (id=141898) [details] > mockups of menus (from Jeremy) Can you get the box + arrow + drop shadow in .svg format say?
(In reply to comment #28) > UI concerns: > > * The menu easily overlaps the first workspace entirely with just 4 workspaces. Is this really an issue? I mean when you open the menu you are focused on it anyway and not in the workspace view.
if you press on an icon with multiple windows, and then release while still over the icon, it leaves the menu popped up
Created attachment 142141 [details] [review] app menu 4 Another checkpoint patch; working on un-regressing drag and drop now.
Created attachment 142258 [details] [review] [dnd] Add API to programmatically initiate a drag For some use cases we have other behavior on mouse press and want to manually control when a drag starts. Split out the drag initiation code into startDrag.
Created attachment 142259 [details] [review] [ShellTextureCache] Add shell_texture_cache_pixbufs_equal Mutter is currently creating separate pixbufs for window icons; use this to analyze them.
Created attachment 142260 [details] [review] [ShellButtonBox] Add shell_button_box_fake_release The application menu code wants to do a popup after a given timeout while holding. We can implement that by adding a function to manually break the grab held by the button box.
Created attachment 142261 [details] [review] Bug 591763 - Add application window menu When we have multiple windows for an application, implement the following behavior: * On click + immediate release, go to the most recently used * On click, hold for 0.7s, pop up a menu with windows, filtering the window list to just those windows. Mouse over on the window list highlights the moused-over window. Implement this by splitting well item into InactiveWellItem and RunningWellItem, sharing a base class BaseWellItem.
Created attachment 142262 [details] [review] [ShellTextureCache] Add shell_texture_cache_pixbufs_equal Mutter is currently creating separate pixbufs for window icons; use this to analyze them.
Comment on attachment 142258 [details] [review] [dnd] Add API to programmatically initiate a drag yeah, i guess that works
Comment on attachment 142260 [details] [review] [ShellButtonBox] Add shell_button_box_fake_release >- set_hover (box, TRUE); >+ /* Special case these two since a hover handler may depend >+ * on the pressed state. >+ */ >+ if (box->priv->held) >+ box->priv->pressed = TRUE; >+ box->priv->hover = TRUE; >+ > if (box->priv->held) >- set_pressed (box, TRUE); >+ g_object_notify (G_OBJECT (actor), "pressed"); >+ g_object_notify (G_OBJECT (actor), "hover"); That comment only makes sense if you're looking at the diff. And a better fix might be to just call g_object_freeze_notify/g_object_thaw_notify around it.
(In reply to comment #36) > Created an attachment (id=142261) [details] > Bug 591763 - Add application window menu I know the UI bits aren't done, but I just want to vote for the white-background menu from the mockup rather than the black one. btw, the description of what alt-tab needs is the combination of the design doc (p22-23) and bug 590563. In particular, we'll need the pop-up window menu (and as currently written, we'll need it extending below the icon, not to the side). But we don't want drag and drop.
Comment on attachment 142262 [details] [review] [ShellTextureCache] Add shell_texture_cache_pixbufs_equal >+static size_t >+pixbuf_byte_size (GdkPixbuf *pixbuf) >+{ >+ return (gdk_pixbuf_get_height (pixbuf) - 1) * gdk_pixbuf_get_rowstride (pixbuf) + >+ + gdk_pixbuf_get_width (pixbuf) * ((gdk_pixbuf_get_n_channels (pixbuf) * gdk_pixbuf_get_bits_per_sample (pixbuf) + 7) / 8); >+} The "height - 1" seems like it must be wrong, but I don't really know what several of the terms there are doing, so I'm willing to believe it's not. Add a comment explaining the math?
(In reply to comment #41) > (From update of attachment 142262 [details] [review]) > >+static size_t > >+pixbuf_byte_size (GdkPixbuf *pixbuf) > >+{ > >+ return (gdk_pixbuf_get_height (pixbuf) - 1) * gdk_pixbuf_get_rowstride (pixbuf) + > >+ + gdk_pixbuf_get_width (pixbuf) * ((gdk_pixbuf_get_n_channels (pixbuf) * gdk_pixbuf_get_bits_per_sample (pixbuf) + 7) / 8); > >+} > > The "height - 1" seems like it must be wrong, but I don't really know what > several of the terms there are doing, so I'm willing to believe it's not. Add a > comment explaining the math? I'd stolen it from gdk_pixbuf_copy; added a comment noting so.
(In reply to comment #41) > (From update of attachment 142262 [details] [review]) > >+static size_t > >+pixbuf_byte_size (GdkPixbuf *pixbuf) > >+{ > >+ return (gdk_pixbuf_get_height (pixbuf) - 1) * gdk_pixbuf_get_rowstride (pixbuf) + > >+ + gdk_pixbuf_get_width (pixbuf) * ((gdk_pixbuf_get_n_channels (pixbuf) * gdk_pixbuf_get_bits_per_sample (pixbuf) + 7) / 8); > >+} > > The "height - 1" seems like it must be wrong, but I don't really know what > several of the terms there are doing, so I'm willing to believe it's not. Add a > comment explaining the math? The logic here is that in some cases - in particular when using rowstride and gdk_pixbuf_new_from_data() to subset a larger pixbuf - the bytes past the end of the image data aren't necessarily valid. (height - 1) * rowstride + width * byte_per_pixel is as far as we have guaranteed image data. I'd do an ascii art diagram of the subsetting case, but am not that inspired.
*** Bug 591705 has been marked as a duplicate of this bug. ***
Created attachment 142434 [details] [review] Bug 591763 - Add application window menu When we have multiple windows for an application, implement the following behavior: * On click + immediate release, go to the most recently used * On click, hold for 0.7s, pop up a menu with windows, filtering the window list to just those windows. Mouse over on the window list highlights the moused-over window. Implement this by splitting well item into InactiveWellItem and RunningWellItem, sharing a base class BaseWellItem.
Created attachment 142445 [details] [review] Add ShellMenuBehavior An object for implementing menu-like items.
Created attachment 142446 [details] [review] Bug 591763 - Add application window menu When we have multiple windows for an application, implement the following behavior: * On click + immediate release, go to the most recently used * On click, hold for 0.7s, pop up a menu with windows, filtering the window list to just those windows. Mouse over on the window list highlights the moused-over window. Implement this by splitting well item into InactiveWellItem and RunningWellItem, sharing a base class BaseWellItem.
Comment on attachment 142445 [details] [review] Add ShellMenuBehavior >+ * SECTION:shell-menu-behavior Not sure I like the name... "behavior" suggests to me that it's not an actor, it just adds behavior to an existing actor. >+enum { >+ PROP_0 >+}; unused >+static gboolean >+shell_menu_behavior_contains (ShellMenuBehavior *box, >+ ClutterActor *actor) I've written this same code in javascript at least twice. Maybe we should try to get it added to ClutterActor. >+ g_signal_connect (box->priv->selected, "destroy", G_CALLBACK(on_selected_destroy), box); >+ g_signal_emit (G_OBJECT (box), shell_menu_behavior_signals[SELECTED], 0, actor); (nitpicky, but it's odd that you use "box->priv->selected" in the first and "actor" in the second when they're the same value. Also, space between G_CALLBACK and (, both here and above.) >+void >+shell_menu_behavior_popup (ShellMenuBehavior *box, >+ guint button, >+ guint32 activate_time) >+{ >+ box->priv->have_grab = TRUE; >+ clutter_grab_pointer (CLUTTER_ACTOR (box)); >+} You don't use button and activate_time (and the js code passes the wrong value for button anyway). Is that just cruft, or do you have a plan to use that info later on?
Comment on attachment 142446 [details] [review] Bug 591763 - Add application window menu >* On click, hold for 0.7s, pop up a menu with windows, filtering >+const WELL_MENU_POPUP_TIMEOUT_MS = 600; mismatch? >+ this._windowContainer = new Shell.MenuBehavior({ orientation: Big.BoxOrientation.VERTICAL, Any reason for "Container" rather than "Menu"? So I guess I see now why ShellMenuBehavior is not "ShellMenu"; it doesn't impose any ideas of layout at all, it just implements the idea of a container widget that has arbitrary "menu items" of some sort inside it... maybe put that into the ShellMenuBehavior docs >+ Shell.draw_box_pointer(texture, WELL_MENU_BORDER_COLOR, WELL_MENU_BACKGROUND_COLOR); we really need to work on cairo introspectability... >+ _onWindowUnselected: function (actor, child) { >+ let transparent = new Clutter.Color(); >+ transparent.from_pixel(0x00000000); >+ child.background_color = transparent; any reason you don't just make a toplevel const TRANSPARENT_COLOR? (Could be used from InactiveWellItem._onPressedChanged too. >+ let window = child._window; >+ this.emit('highlight-window', null); >+ }, don't need the "window" variable there >diff --git a/src/Makefile.am b/src/Makefile.am >index 9dd9ff8..d4d5678 100644 >--- a/src/Makefile.am >+++ b/src/Makefile.am >@@ -75,6 +75,8 @@ libgnome_shell_la_SOURCES = \ > shell-generic-container.h \ > shell-gtk-embed.c \ > shell-gtk-embed.h \ >+ shell-menu-behavior.c \ >+ shell-menu-behavior.h \ > shell-overflow-list.c \ > shell-overflow-list.h \ > shell-process.c \ oops, this belongs in the previous patch
(In reply to comment #48) > (From update of attachment 142445 [details] [review]) > >+ * SECTION:shell-menu-behavior > > Not sure I like the name... "behavior" suggests to me that it's not an actor, > it just adds behavior to an existing actor. It is kind of a change from ShellButtonBox; I think I'd prefer that one to be Behavior now too. Well, I guess the most expressive would be BehaviorBox but that's getting a bit long =) > > >+static gboolean > >+shell_menu_behavior_contains (ShellMenuBehavior *box, > >+ ClutterActor *actor) > > I've written this same code in javascript at least twice. Maybe we should try > to get it added to ClutterActor. Can't really since ClutterActor isn't a container. But I hear a concrete base class is going to be added to clutter, and it would make sense there. (And possibly move higher-level events into there as well). > You don't use button and activate_time (and the js code passes the wrong value > for button anyway). Is that just cruft, or do you have a plan to use that info > later on? Was copying from GtkMenu in the theory that if we evolved the MenuBehavior it might be useful.
Closing this as it's committed now, further improvements can be new bugs.