GNOME Bugzilla – Bug 594699
0001--AppWell-Allow-popup-menu-to-be-persistent-and-sup.patch
Last modified: 2009-09-14 19:49:26 UTC
Created attachment 142843 [details] [review] 0001--AppWell-Allow-popup-menu-to-be-persistent-and-sup.patch 0001--AppWell-Allow-popup-menu-to-be-persistent-and-sup.patch
Review of attachment 142843 [details] [review]: There seems to be three independent things in this patch: 1) The menu release UI behavior 2) the window selection UI behavior 3) a tweak to the animation back to the workspaces Not sure how I feel about the menu release UI behavior. The big pro is that click and drag is hard with a stylus device or finger because you can easily lose the drag for a second. But that's equally true with drag and drop as well - and maybe drag actions just need to be modified for stylus/finger devices - once you begin a drag it sticks and yuo have to click to end it. (So, here the menu would always be sticky on a tablet device.) Without this patch the behavior slotted neatly into the normal GTK+ menu behavior for using menus in drag mode - you just had to hold down to get it. With this behavior it's almost backwards from the normal GTK+ menu behavior which is that if you click and release in less than 0.5 seconds, the menu stays up. I have two concerns with this: - People used to normal menus, will thing that the menu should cancel if they click hold, the menu comes up, they look at it a bit and release - Mistraining - you click, get a menu, release it stays up, your brain says "ah, click and release gives me that menu", you try it again, and boom, you get taken to some window. I had several problems testing this where despite understanding how it should work clicked and released without waiting. ::: js/ui/appDisplay.js @@ +654,3 @@ }, + _findWindowCloneForActor: function (actor) { Why not just add the clone._delegate as we do elsewhere for the back-pointer (in workspaces.js, not here), with that and instanceOf you probably can get rid of cachedWindowClones entirely. @@ +666,3 @@ + // This function is called while the menu has a pointer grab; what we want + // to do is see if the mouse was released over a window clone actor + _onMenuRelease: function (actor, event) { Just spell spell out _onMenuButtonRelease @@ +861,3 @@ + + // If we successfully picked a window, don't reset the workspace + // state, since that causes visual noise. The workspace gets This reasonable but has nothing to do with the rest of the patch? And maybe the other windows should be shown anyways (with the new stacking order) and just not resize these windows back to their original size - to avoid the effect that you go back to the normal view, then stuff suddenly flashes in. ::: js/ui/overview.js @@ +380,3 @@ + */ + lookupCloneForWindow: function (metaWindow) { + return this._workspaces.lookupCloneForMetaWindow(metaWindow); I'm not sure I quite understand why we are "lasagna-code" duplicating methods from Workspaces on Overview ::: src/shell-menu.c @@ +36,2 @@ static gboolean +shell_menu_contains (ClutterContainer *container, If you are generalizing, this should get renamed to container_contains() or something @@ +182,3 @@ + g_signal_handlers_disconnect_by_func (G_OBJECT (box->priv->source_actor), + G_CALLBACK (on_source_destroyed), + box); I don't see where you disconnect when the menu is disposed - the menu's ->dispose() should probably set_persistent_source(NULL);
(In reply to comment #1) > > 1) The menu release UI behavior > 2) the window selection UI behavior These two I see as relatively intertwined. Possible to separate but I'd rather not. > 3) a tweak to the animation back to the workspaces True, I could break this one out? > With this behavior it's almost backwards from the normal GTK+ menu behavior > which is that if you click and release in less than 0.5 seconds, the menu stays > up. Hm, it should be that if you click and release in less than 0.5 you won't see a menu, we pick the most recent window, no? > ::: js/ui/overview.js > @@ +380,3 @@ > + */ > + lookupCloneForWindow: function (metaWindow) { > + return this._workspaces.lookupCloneForMetaWindow(metaWindow); > > I'm not sure I quite understand why we are "lasagna-code" duplicating methods > from Workspaces on Overview The reason I started proxying originally was because the overview has a conceptual hand in some of this stuff, and it seemed a bit cleaner at the time than .getWorkspaces(). But we could certainly do that now that there's a non-small number of methods.
Created attachment 143025 [details] [review] Don't proxy methods through overview, just add a getWorkspaces() It was just unnecessary.
Created attachment 143026 [details] [review] [ShellMenu] Make popdown,popup methods idempotent; emit activate before popdown Callers will generally expect _popup and _popdown to be a no-op if the menu is already in that state; make it so. Also emit the 'activate' signal before we pop down, since that's much more convenient for most uses of menu callbacks which want to do something different on activate+popdown than just popdown.
Created attachment 143027 [details] [review] Allow popup menu to be persistent, and support direct window selection When the user click+hold+release over the icon, the effect we want is for the menu to stick around. Also, allow the user to mouse over the actual windows and select them directly. If the user mouses over a window, reflect that in the menu.
Created attachment 143028 [details] [review] [AppWell] When in window filtering mode, reset filter before showing window When we had a filtered set of windows, and want to exit the overview into a particular window, the cleanest way to do this is to split the animation time in half; half goes to resetting the overview to its unfiltered state, then half goes to picking the window. To implement this, add public functions which either take an animation time, or add an internal variant if it's just used internally.
With these new 4 patches, basically the first two are cleanup, the third brings us the click->hold->release = stay-popped up behavior, and the 4th cleans up the leaving-overview animation transition from a filtered window state.
Review of attachment 143025 [details] [review]: Ugh, just thinking about it, I think this is wrong - it's not going to make sense once Dan has Workspaces-objects-per-monitor. (I think that's the plan - unless there is a plan to switch it to a two-level Workspaces/WorkspacesGrid or something. Sorry for the dead-end suggestion.
(In reply to comment #8) > Review of attachment 143025 [details] [review]: > > Ugh, just thinking about it, I think this is wrong - it's not going to make > sense once Dan has Workspaces-objects-per-monitor. (I think that's the plan - > unless there is a plan to switch it to a two-level Workspaces/WorkspacesGrid or > something. Sorry for the dead-end suggestion. Hmm, so inside the overview we'd look up the Workspace object for a given window depending on what monitor it's on?
Review of attachment 143026 [details] [review]: I think it's wrong to emit ::activate when the menu is still up - what if the activated action wants to get a clutter grab? I think it would be cleanest to separate into ::activate and ::cancelled and get rid of the generic ::popdown.
Review of attachment 143027 [details] [review]: Bit of leftover, noted below. Otherwise looks good. I still think this is two completely independent patches, (you even said "also" in the commit message...), but that's OK. ::: js/ui/overview.js @@ +389,3 @@ + * which represents it in the workspaces display. + */ + lookupCloneForWindow: function (metaWindow) { Doesn't seem that you use this any more (or the corresponding change to workspaces.js)
(In reply to comment #9) > (In reply to comment #8) > > Review of attachment 143025 [details] [review] [details]: > > > > Ugh, just thinking about it, I think this is wrong - it's not going to make > > sense once Dan has Workspaces-objects-per-monitor. (I think that's the plan - > > unless there is a plan to switch it to a two-level Workspaces/WorkspacesGrid or > > something. Sorry for the dead-end suggestion. > > Hmm, so inside the overview we'd look up the Workspace object for a given > window depending on what monitor it's on? Yeah (so Workspaces and Workspace are about the GUI which has a workspace grid per monitor, and not about the underlying MetaWorkspace objects which are as before.) The other option would be to have getWorkspaces(window) - which would avoid the lasagna-pollution of Overview with lots of Workspaces methods.
(In reply to comment #12) > > The other option would be to have getWorkspaces(window) - which would avoid the > lasagna-pollution of Overview with lots of Workspaces methods. I like this option, will make a patch.
Review of attachment 143028 [details] [review]: I don't like the effect of quickly shrinking the window down and growing it again at all, very abrupt, very visually noisy. Did you try my suggestion?
Created attachment 143037 [details] [review] Don't proxy methods through overview; add a getWorkspacesForWindow() Duplicating the methods was unnecessary. Also, we want a getWorkspacesForWindow() method as preparation for multi-monitor work.
Created attachment 143038 [details] [review] [ShellMenu] Make popdown,popup methods idempotent; remove 'popdown' for 'cancelled' Callers will generally expect _popup and _popdown to be a no-op if the menu is already in that state; make it so. Also change the 'popdown' signal to be 'cancelled'; this is clearer and allows us to avoid having activate also call popdown.
Created attachment 143039 [details] [review] Allow popup menu to be persistent, and support direct window selection When the user click+hold+release over the icon, the effect we want is for the menu to stick around. Also, allow the user to mouse over the actual windows and select them directly. If the user mouses over a window, reflect that in the menu.
Created attachment 143040 [details] [review] [AppWell] When in window filtering mode, reset filter before showing window When we had a filtered set of windows, and want to exit the overview into a particular window, what we do is re-show all the old windows first, but don't reset the scaling on them. This will involve some overlapping, but that's not a big deal because we'll immediately get overlap anyways in the normal case zooming the windows back.
Should address all comments. I spent quite a while trying to make the stacking order work right but ran into multiple bugs, and didn't want to scope creep this patch too much. The main issue is the mouseover handler in WindowClone, but there are other ones; I think the lightboxing may not be quite right.