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 584609 - Zooming the whole overlay in transitions
Zooming the whole overlay in transitions
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2009-06-02 12:52 UTC by Sander Dijkhuis
Modified: 2009-08-11 00:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch for zooming the whole overlay (13.98 KB, patch)
2009-07-11 22:23 UTC, Sander Dijkhuis
reviewed Details | Review
Updated patch (15.38 KB, patch)
2009-07-17 00:32 UTC, Sander Dijkhuis
none Details | Review
Updated with lineair window movement (18.55 KB, patch)
2009-08-06 22:15 UTC, Sander Dijkhuis
none Details | Review
Rebased patch (18.38 KB, patch)
2009-08-08 12:23 UTC, Sander Dijkhuis
none Details | Review
Rebased and some changes (18.36 KB, patch)
2009-08-10 15:03 UTC, Sander Dijkhuis
none Details | Review
With Dash fading (18.36 KB, patch)
2009-08-10 23:03 UTC, Sander Dijkhuis
none Details | Review
The right patch (18.97 KB, patch)
2009-08-10 23:39 UTC, Sander Dijkhuis
committed Details | Review

Description Sander Dijkhuis 2009-06-02 12:52:40 UTC
Currently when showing or hiding the overlay, only the workspaces are moving
and changing size. It might be better to give the whole overlay a zooming
animation, while keeping the workspaces fixed on the overlay plane. I think
this gives the user a better mental model of where the overlay lives when
hidden, and removes the amount of objects confusingly sliding around.

It also seems to come close to what Jeremy Perry describes on his mockup page
for application browsing:
> When the user first opens this scene, the animation could give the sense
> that you've pulled back, so the desktop recedes to the right, and the items
> on the left appear to come in from above left
http://live.gnome.org/GnomeShell/DesignerPlayground/AppBrowsingAlternative

I started playing with the idea in the 'zoom' branch at:
http://github.com/sander/gnome-shell/tree/zoom
The code is currently very ugly and it only works when the first workspace is
activated, but it might already give a good idea of how it would look.

One concern mentioned at IRC is that the huge labels in the menu would look
too bad. I didn't find it disturbing, especially with ANIMATION_TIME = 0.25
as proposed in #583572.
Comment 1 Owen Taylor 2009-06-04 22:00:51 UTC
I rather like the effect - with the shorter time and the EaseInQuad animation it's pretty hard to see the initial portions where the text would be large.

It's a bit hard to evaluate though without fixing up code that makes the windows move linearly on the transition since it's pretty disturbing to have windows swooping around as you go in.

Comment 2 Sander Dijkhuis 2009-06-04 23:35:47 UTC
With 5 workspaces, the text becomes really large, but that's probably not a use case that should be optimized for. And you still don't get to see the text pixelated.

What do you mean by the windows moving linearly? Both the workspace and the windows currently have an easeOutQuad transition, so it seems their movements correspond linearly. (That's higher relativity theory though, so I might be wrong on this time of the day.) Just setting the windows' transition to "linear" doesn't improve the situation. For further experimenting, see Workspace._positionWindows() in workspaces.js.
Comment 3 Dan Winship 2009-06-05 00:28:35 UTC
(In reply to comment #2)
> What do you mean by the windows moving linearly?

So, imagine that you have four workspaces, and you're on workspace 1, and you have a window in the top-right corner of the screen, which the overlay is going to place in the bottom-left corner of the exposé-ed workspace in the overlay. But since there are 4 workspaces, the bottom-left corner of the first workspace will end up being only halfway down the screen, right?

What you want is for the window to uniformly move from its original position at the top-right directly to its final position at center-left. But if you move the windows independently of the workspaces, then what will happen is that the window will start moving down and to the left, but the workspace will simultaneously be moving upward and shrinking, and eventually the downward motion of the window will be scaled to the point where it's less than the upward motion of the workspace, and so the overall path of the window as you enter the overlay is U shaped instead of linear. And when you have lots of windows doing this, it's sort of confusing.

The current code uses the workspaceRelative tweener hack at the end of overlay.js to compensate for this by actually moving the window in a curve rather than a straight line, so as to exactly cancel out the effect of moving and shrinking the workspace. I haven't looked at your code, but if this isn't working any more, then probably it's because you changed the way that the workspace moves, but didn't change workspaceRelative as well, and so now it's compensating incorrectly.
Comment 4 Sander Dijkhuis 2009-07-11 22:23:35 UTC
Created attachment 138252 [details] [review]
Patch for zooming the whole overlay

Thanks a lot for the clear explanation! I think I've got that issue fixed now by making sure that workspaceRelative uses the correct workspace position. I'm not sure whether I should change the window transition curve too, but keeping  easeOutQuad looked best to me.

Zooming in and out from other workspaces than the first one was easily implemented by using the right workspace positioning variables (gridX instead of fullSizeX).

The attached patch is for the master branch, and obsoletes my original Git branch.
Comment 5 Owen Taylor 2009-07-16 00:36:26 UTC
Mostly the patch looks really good to me. Few small comments:

 - Instead of returning a dictionary of x,y for the position, maybe
   return an array [x, y] ? With Spidermonkdy, you can do:

    posX, posY = someFunctionThatReturnsAnArray()

   We don't do a lot of either right now so I can't really say
   definitively what our "style" is for this.

   (Workspace._computeWindowPosition() returns an array of [x,y,scale], 
   so it's sort of similar.)

 - Did you mean to remove Workspaces._updateInOverlay? It looks like
   the link between the Workspaces.updatePosition() function you removed
   as no-longer-used, and the Workspace.updateInOverlay function you removed
   as no-longer-used.

   These removals could use explicit commenting in the commit message.

 - I think the code that fades in the remove button could use a comment
   explaining why we need to fade it in (to avoid it appearing huge
   initially?)

 - You probably should removeTweens before fading in the remove button;
   right now it looks like you'll sometimes get competing tweens. If 
   they have the same time, it probably won't be apparent, but maybe
   we'd use different times for the remove button.

And the workspaceRelativeModifier code looks a little finish; I believe what you have ends up as a no-op since workspace.actor.x/y remain constant.

Let:

 w_x, w_y: The position of the workspace actor (fixed during the animation)
 overlay_x0, overlay_y0: The initial position of the overlay actor
 overlay_scale0: The initial scale of the overlay actor
 overlay_x1, overlay_y1: The final position of the overlay actor
 overlay_scale1: The final scale of the overlay actor

The initial screen X position of the window actor is:

 (begin + w_x) * overly_scale0 + overlay_x0

The final screen X position is:

 (end + w_x) * overly_scale1 + overlay_x1

If we interpolate linearly, the screen position at time t is:

 screen_x =
 (1-t) * [(begin + w_x) * overly_scale0 + overlay_x0] +
     t * [(end   + w_x) * overly_scale1 + overlay_x1]

The position in workspace coordinates of the window is then:

 x = (screen_x - overlay.actor.x) / overlay.actor.scale - w_x

[ That, I think, gets the actors upper left corner to move in a straight line. We probably really want the center to move in a straight line. Which could be accomplished with actor.move_anchor_point_from_gravity(). Not sure if the difference would be visible. ]

One other thing to think about - for actors not on the active workspace do 
we want them to just stay in their "spread out" positions and not animate as we zoom in or out?
Comment 6 Dan Winship 2009-07-16 04:44:33 UTC
(In reply to comment #5)
>  - Instead of returning a dictionary of x,y for the position, maybe
>    return an array [x, y] ? With Spidermonkdy, you can do:
> 
>     posX, posY = someFunctionThatReturnsAnArray()
> 
>    We don't do a lot of either right now so I can't really say
>    definitively what our "style" is for this.

You have to say

    [posX, posY] = ...

I think the returning-an-array is implicitly our style, since that's what gjs does for multiple-out-args, and we already have stuff like:

    [width, height] = actor.get_transformed_size()

>  - I think the code that fades in the remove button could use a comment
>    explaining why we need to fade it in (to avoid it appearing huge
>    initially?)

IIRC it's because the same code gets used when you remove a workspace, and it has to add a remove button to the newly-last workspace.

> [ That, I think, gets the actors upper left corner to move in a straight line.
> We probably really want the center to move in a straight line. Which could be
> accomplished with actor.move_anchor_point_from_gravity(). Not sure if the
> difference would be visible. ]

as noted in bug 571109 comment 16 there seem to be problems with changing anchor points in the overlay
Comment 7 Sander Dijkhuis 2009-07-17 00:32:22 UTC
Created attachment 138565 [details] [review]
Updated patch

Thanks for the comments! I have to catch a bus, so had to quickly implement the changes. The mathematical proof looks convincing, though I don't have time to check, but the animation looks the same with the workspaceRelative code removed indeed.

If there are any issues with my patch, feel free to modify it. Otherwise, please commit it. See you in 2.5 weeks! :-)
Comment 8 Owen Taylor 2009-07-17 19:31:43 UTC
My math wasn't meant to prove that what you have was a no-op - I just noted that from inspection. It was meant to be the non-noop-code that is actually needed to get windows to move linearly in screen coordinates.
Comment 9 Sander Dijkhuis 2009-08-06 22:15:28 UTC
Created attachment 140070 [details] [review]
Updated with lineair window movement

Oops, that's what I get when coding in a hurry... This patch implements your new window movement formulas. I agree with them now and it seems like they really work too.

This updated patch re-introduces workspaceRelative with the new math and adds Overlay.getZoomedIn[Scale|Position]() and Overlay.get[Scale|Position]() functions for use in workspaceRelative.

I needed to delay positionWindows() until after Workspaces has been created, because it depends on Main.overlay._workspaces. For this I added a 'workspaces-created' signal to Overlay.
Comment 10 Marina Zhurakhinskaya 2009-08-07 20:00:33 UTC
It looks like this patch needs to be rebased on top of 
http://git.gnome.org/cgit/gnome-shell/commit/?id=4d52c1095816bfba51715b5e836f29483960ea13

I had to apply it manually ( I was too curious to try it with the hot corner :), and then the application icons were not positioned correctly when the overlay was first entered. They did get re-positioned correctly as workspaces were added or windows were moved around. 
Comment 11 Sander Dijkhuis 2009-08-08 12:23:11 UTC
Created attachment 140193 [details] [review]
Rebased patch

I didn't notice the icon positioning bug you described after doing this rebase and changing some things, so I hope that's fixed now.

In this patch I also removed the new 'workspaces-created' signal in favor of connecting to 'showing', and changed the initial workspaceRelative positions in order to improve the cancel-opening-the-overlay animation.
Comment 12 Sander Dijkhuis 2009-08-10 15:03:32 UTC
Created attachment 140335 [details] [review]
Rebased and some changes

The patch needed to be rebased to 06d17e08c06acb3168750251e85e76c51171a1b5

I also applied the change to _workspaceRelativeGet as suggested by Owen on IRC, to interpolate the overlay's dimensions instead of relying on Overlay.getPosition() and Overlay.getScale().

I removed one trailing comma that was in the previous patch after
+                             overlayScale: overlayScale } }
Comment 13 Sander Dijkhuis 2009-08-10 23:03:59 UTC
Created attachment 140392 [details] [review]
With Dash fading

During the transitions, Dash is faded in and out now, based on a mockup by Jeremy and Jon:
http://www.gnome.org/~mccann/shell/mockups/20090810/shellzoom4.html
Comment 14 Sander Dijkhuis 2009-08-10 23:39:10 UTC
Created attachment 140396 [details] [review]
The right patch

The previous patch accidentally was the before-previous patch, this is the described one.
Comment 15 Owen Taylor 2009-08-11 00:15:22 UTC
Behavior and patch look great. Good to commit!

+    // Returns the scale of the overlay when zoomed in to a workspace.

This is just a bit unclear to me. Maybe:

// Returns the scale the overlay has when we just start zooming out
// to overview mode. That is, when just the active workspace is showing.

+        // Position/scale the desktop windows and their children after the
+        // workspaces have been created. This cannot be done first because
+        // window movement depends on the Workspaces object being accessible
+        // as an Overlay member.
+        Main.overlay.connect('showing',
+                             Lang.bind(this, function() {
+            for (let w = 0; w < this._workspaces.length; w++)
+                this._workspaces[w].zoomToOverlay();
+        }));

This is just a bit convoluted, but I don't have a better easy suggestion, so
it's OK. If we go to something like bug 586549, then we'll want to split the
workspaces constructor and workspaces.zoomtoOverlay(), and then this becomes
trivial.

Comment 16 Sander Dijkhuis 2009-08-11 00:24:56 UTC
Made the comment change and committed. Thanks!