GNOME Bugzilla – Bug 737534
Frequent apps overview allows changing workspaces
Last modified: 2014-11-12 19:06:22 UTC
First I notice that using <Super><A> to access frequent apps view and then scrolling with the mouse was changing the workspaces behind. I couldn't change the workspaces with scroll if it was opened in two steps (<Super> and then <Super><A>), but later I found that using <Super><PgUp/PgDn> was still changing workspaces. Allowing to change workspaces during frequent apps view I think is not good, because there is no visual warning of that change and the user will get confused as to why the apps in the desktop are different. Steps to reproduce: 1. Setup gnome-shell to frequent in apps view. 2. Open frequent apps view with <Super><A>. 3. Scroll down with mouse. 4. Use <Super> to return to the desktop. It will be one workspace below from where you were. Steps to reproduce: 1. Setup gnome-shell to frequent in apps view. 2. Open Overview with <Super>. 3. Open frequent apps view with <Super><A>. 4. Use <Super><PgDn>. 5. Use <Super> to return to the desktop. It will be one workspace below from where you were.
(In reply to comment #0) > [...] > Steps to reproduce: > 1. Setup gnome-shell to frequent in apps view. > 2. Open Overview with <Super>. > 3. Open frequent apps view with <Super><A>. > 4. Use <Super><PgDn>. This is actually reproducable with 1. go to app picker 2. Use <Super><PgDn> e.g. it doesn't matter how the overview was entered or whether showing frequent or all apps. Ugh, we should definitively fix that ...
Created attachment 290062 [details] [review] Proposed patch It's not a nice way of getting around the problem, but it works in my testing. It uses an extension-style approach by making a temporary injection into Meta.Workspace when the active view is not the workspaces page (ie. it is either showing search results or the app picker). It currently adds more code to _showPage, but this could be split out into its own function. There is also an emit in the _showPage function, so the logic in this patch could be moved into the Overview or OverViewControls class with a signal connect on 'page-changed' if that would make more sense. Maybe a change to Meta.Workspace in Mutter might be a better solution?
As an example of moving the logic to another class and connecting the signal: https://github.com/N-Yuki/gnome-shell-scroll-workspaces/commit/47c80c32f4b536712a21621cc78822b8b2d4034a
Review of attachment 290062 [details] [review]: No. This is fine in an extension, but we do not need to resort to hacks like this in core. For the keyboard-shortcuts, we could add some simple inhibitWorkspaceSwitch()/uninhibitWorkspaceSwitch() API to WindowManager. However on second thought, I'm no longer that convinced that this is a bug - the shortcuts are a user action, they are not really ambiguous and I can think of valid use cases for them ("<super>a, remember you'd rather launch on a new workspace, <super>end" instead of "<super>a, remember ..., <esc>, <super>end, <super>a", multi-monitor system with workspaces-only-on-primary set to false, ...). The scrolling issue is different and something we should fix.
(In reply to comment #4) > The scrolling issue is different and something we should fix. So WorkspacesView is tries to handle with this[0], but it fails when opening the app switcher directly because its actor is not actually hidden in that case (only faded to opacity 0) ... [0] https://git.gnome.org/browse/gnome-shell/tree/js/ui/workspacesView.js#n657
I can't seem to reproduce the scrolling issue with 3.12.2, but there hasn't been too much change in WorkspacesView between 3.12.2 and master.
Well, the way we switch to the overview when going straight to the app picker is completely different in 3.14.x. In 3.14 and master, the scrolling issue described in comment #0 is easily reproducable.
Created attachment 290208 [details] [review] Proposed patch The way it's currently handled seems a bit weird, but anyway, I added a call to hide() on top of setting the opacity to 0. This patch feels kinda incomplete though (animateFromOverview might need a change too). I haven't tested it, so I'm not sure if it actually works.
Review of attachment 290208 [details] [review]: ::: js/ui/viewSelector.js @@ +197,2 @@ this._workspacesPage.opacity = 0; + this._workspacesPage.hide(); I had a similar patch locally (just in workspacesView), but not a fan of either approach. I'll attach a patch I like better in a bit, I'd appreciate if you took a look.
Created attachment 290215 [details] [review] viewSelector: Hide pages originally Only a single page is supposed to be visible when not animating between pages, so the previously active page is hidden at the end of the transition. However when showing the first page (i.e. when entering the overview), there is no previously active page and all pages are visible. This causes issue in the workspacesDisplay when starting out with the app picker, as it relies on being hidden to disable workspace switches by scrolling or panning. By hiding all pages originally, we fix the original assumption of only having a single page visible at a time and with that all the issues caused by breaking it.
Review of attachment 290215 [details] [review]: If that change solves the problem, then it's a clean solution, but I don't see why that would actually work. Just after construction (well, there's an if-else block first), there is: page.hide(); which will set 'visible' to false from my understanding (https://git.gnome.org/browse/clutter/tree/clutter/clutter-actor.c#n1659). Basically, _addPage should already be constructing pages that are not visible, so if this patch corrects the problem, then there might be something weird happening unless there's a difference here between setting the visibility to false on construction and setting it to false via hide(). Would it have something to do with what Shell.Stack.add_actor() does? Additionally, 'visible' is by default set to false in Clutter 1.20.0 (latest stable): https://git.gnome.org/browse/clutter/tree/clutter/clutter-actor.c?id=1.20.0#n6707 This however seems to be changed to a default of true in master: https://git.gnome.org/browse/clutter/tree/clutter/clutter-actor.c#n6024
That default seems to be wrong, or at least misleading. Setting visible to FALSE, even if it's already FALSE, will then set the show-on-set-parent property to FALSE, which is by default TRUE: https://git.gnome.org/browse/clutter/tree/clutter/clutter-actor.c?id=1.20.0#n1804 Which is exactly what's being triggered here. You're right that this shouldn't make a difference from the explicit hide a few lines later, though.
Mmh, indeed - must have been hovering over some app picker padding when testing this. I fixed up the commit message of your patch and pushed, let's close this ...