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 737534 - Frequent apps overview allows changing workspaces
Frequent apps overview allows changing workspaces
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: overview
3.14.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2014-09-28 16:02 UTC by Eddy Castillo
Modified: 2014-11-12 19:06 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Proposed patch (1.24 KB, patch)
2014-11-06 00:01 UTC, Yuki N.
rejected Details | Review
Proposed patch (1.12 KB, patch)
2014-11-08 06:17 UTC, Yuki N.
committed Details | Review
viewSelector: Hide pages originally (1.45 KB, patch)
2014-11-08 14:01 UTC, Florian Müllner
rejected Details | Review

Description Eddy Castillo 2014-09-28 16:02:24 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.
Comment 1 Florian Müllner 2014-09-28 16:13:28 UTC
(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 ...
Comment 2 Yuki N. 2014-11-06 00:01:26 UTC
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?
Comment 3 Yuki N. 2014-11-07 10:34:52 UTC
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
Comment 4 Florian Müllner 2014-11-07 12:43:02 UTC
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.
Comment 5 Florian Müllner 2014-11-07 12:50:42 UTC
(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
Comment 6 Yuki N. 2014-11-07 14:54:35 UTC
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.
Comment 7 Florian Müllner 2014-11-07 15:08:19 UTC
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.
Comment 8 Yuki N. 2014-11-08 06:17:24 UTC
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.
Comment 9 Florian Müllner 2014-11-08 13:51:04 UTC
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.
Comment 10 Florian Müllner 2014-11-08 14:01:50 UTC
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.
Comment 11 Yuki N. 2014-11-08 14:51:05 UTC
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
Comment 12 Jasper St. Pierre (not reading bugmail) 2014-11-08 16:19:58 UTC
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.
Comment 13 Florian Müllner 2014-11-12 19:06:12 UTC
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 ...