Created attachment 140884 [details] [review]
Adds zooming by scrolling.

This should fix it for now. The only things that could need improvement are the positioning of the window icons and the zoom out on mouse leave. (There is also a bug when moving the mouse on zooming out shows the title to be small for a few seconds, but this could be fixed by adding another boolean to _updateTitle).

This is a second patch with an alternate approach. Instead of zooming full on mouse-up, it zooms in fully on five mouse-wheels. To quickly go back to the original activities overview, right-click on the window clone actor.

Created attachment 140900 [details] [review]
Adds zooming by scrolling (approach 2).

*** Bug 591989 has been marked as a duplicate of this bug. ***

Sorry it took me a few days to get to these. Good job on these! it's nice to see someone paying attention to details and trying out different ways things can work.

I think of the two different patches, I like the second one better - with the animation lag the first one feels heavyweight. And zooming all the way in on a maximized window looks very visually similar to leaving the overlay.

In terms of how far it should zoom with one click, the basic factors I see are:

 - The first click should give me enough of a zoom to be usefully bigger than the initial size
 - I should be able to get in as far as I want without having to lift my finger to scroll some more

And the 5 clicks steps you have seem to work well for that.

Some observations about the UI:

 * I don't think anybody is going to find right click to unzoom; I'm not sure it has much value. It doesn't feel like an inverse of scrolling up to zoom in.

 * I think the zoom should end when you mouse out - it's annoying right now that you have to zoom all the way down to to a zoom of zero to get out of the mode. After zooming in on one window (and maybe deciding it wasn't the one I wanted), as soon as I zoom down enough to see some other window I want to zoom in on or click to activate, I should be able to go to that window and interact with it. (Once you left a window the one you left could tween back to its original size smoothly.)

 * How you are handling the positioning as you zoom in seems to work for well for maximized clients, but it's a little strange for me for smaller windows -  the window leaves where the mouse cursor is and heads toward the center of the screen.

Some observations about the patch (generally looks very good):

+const OVERLAY_COLOR = new Clutter.Color();
+OVERLAY_COLOR.from_pixel(0x000000AA);

Needs a comment as to what it's being used for.

+        Main.overview.connect('hidden', Lang.bind(this, function(a, event) { if (this.actor.step > 0) this._zoomEnd(this.actor, event); } ));

It would feel more natural to me do this in zoomStart and disconnect it on zoomEnd. (You'll need to save the "id" of the signal connection to disconnect - say as this._overlayHiddenId)

Also, while we don't have fixed hard limits for line length, around 100 characters is usually about as wide as it makes sense.

+        if (direction == Clutter.ScrollDirection.UP) {
+            if (actor.step == undefined)
+                this._zoomStart(actor, event);
+            if (actor.step < 1.0) {
+                actor.step += SCROLL_SCALE_AMOUNT;
+                this._zoomUpdate(actor);
+            }
+        } else if (direction == Clutter.ScrollDirection.DOWN) {
+            if (actor.step > 0.0) {
+                actor.step -= SCROLL_SCALE_AMOUNT;
+                actor.step = Math.max(0, actor.step);
+                this._zoomUpdate(actor);
+            }
+            if (actor.step <= 0.0)
+                this._zoomEnd(actor, event);
+        }

This assumes that both:

  0.2 + 0.2 + 0.2 + 0.2 + 0.2 >= 1.0

and that 

  0.2 + 0.2 + 0.2 + 0.2 + 0.2 - 0.2 - 0.2 - 0.2 - 0.2 - 0.2 <= 0

Avoiding those type of assumption is usually a good idea when it comes to floating point numbers. I think it might work better to have an integer this._zoomLevel that is 0,1,2,3,4,5, and then compute things from that.

While assigning properties to the Javascript ClutterActor proxy object -
'actor.step = 1' or whatever - is possible, we tend to do it only as a last resort. If ClutterActor had a property added to it in some future version of Clutter called 'step', then your code would suddently do something very different.

Usually, as in this case, we have an Javascript application object - the 'delegate' - and write our code and set our properties on that. For things like drag-and-drop where we need to be able to go from the ClutterActor to the delegate, we set exactly one property on the actor - '_delegate' which points to the delegate Javascript object. (Doesn't seem necessary for what you are doing - you always have the delegate, so you don't have to backtrack from the actor to get it.)

So, you can just use this._<whatever> rather than actor.whatever. (The _ marks the property as being private to the implementation of the JS object.

+        [actor.x, actor.y, actor.scale_x, actor.scale_y] = [actor.globalOrig[i] + (actor.target[i] - actor.globalOrig[i]) * actor.step for (i in [0, 1, 2, 3])];

IMO, a little too tricky and using 4 element arrays as structures is a little hard to read. (Though I'm really impressed by how compact your patch is! Your trick is elegant, but our overall style is a bit more plodding :-)

Maybe something like:

    _zoomUpdate : function (actor) {
       function interpolate(start, end) {
           /* compute factor from 0-1 out of this._zoomLevel */
           return (1 - factor) * start + (factor * end);
       }
       actor.x = interpolate(this._zoomStartX, this._zoomTargetX);
       actor.y = interpolate(this._zoomStartY, this._zoomTargetY);
       [...]
    }

+        global.overlay_group.add_actor(this._overlay);

The fact that the dash and workspaces dim but the top bar doesn't seems a little odd to me. Would it be better to just add this to the stage and dim everything?

+        actor.reparent(global.stage);
+        this._showTitle();
+        this._title.reparent(global.stage);

The need to reparent out is non-obvious enough that you should have a comment explaining why you are doing it.

And a couple of observations about git and patches in general:

 - It's generally nicer to commit your patches to git (using git add and git commit), use 'git format-patch' and attach that. My tool 'git bz' (http://blog.fishsoup.net/2008/11/16/git-bz-bugzilla-subcommand-for-git/) makes this really easy, but you can also do it manually.

 - Once you've attached a patch, and then made some more fixes, you will frequently want to use add your changes and then 'git commit --amend' to merge them into your previous patch.

 - If before you commit you do a 'git diff' and you notice that you have whitespace changes or other unintentional changes, you can use 'git add -p' to interactively add just some parts of the changes. This can also be used to split a bunch of uncommitted changes into multiple patches. Once you have everything you want save committed, you can use 'git checkout .' to wipe out remaining uncomitted changes (like whitespace changes.)

Thanks for that really nice overview.

I'm typically a Pythonista so the first thing I did when I came to JavaScript was look for some form of multiple variable assignment (called tuple unpacking in Python) and list/array comprehensions.

https://developer.mozilla.org/en/New_in_JavaScript_1.7#Destructuring_assignment
https://developer.mozilla.org/en/New_in_JavaScript_1.7#Array_comprehensions

These types of expressions make code really compact, but I'll see what I can do about the more explicit style guide you follow..

When I get home I'll work on making a better patch, with not so much float abuse. (I did have some trouble with that while testing it, so I think the _zoomLevel approach should be nicer. I may even do a 0-100 scale)

Couple of other questions:

 - Do I *need* to reparent the actors? I did that before I found out about transformedPosition and transformedSize.
 - How should we best zoom in if we can't zoom in to the center of the screen? I saw the problem with the mouse cursor, but I think the best approach would be to attach a scroll-event handler to the overlay in _zoomStart.
 - I removed the zoom end on mouse because if you zoomed in a small window you couldn't zoom, in further because your mouse moving would cause it to leave scroll view. Maybe there should be an enter handler that sets a leave handler if there wasn't one already, but it gets reset on every click of the scroll wheel. Would that be a good solution?
 - I like the overlay not covering the dash because you can mouse over Activities and gets out of that mode.

Also, thanks for the git tips, didn't know about format-patch and --append, I'm still a git newbie.

(In reply to comment #6)
> Thanks for that really nice overview.
> 
> I'm typically a Pythonista so the first thing I did when I came to JavaScript
> was look for some form of multiple variable assignment (called tuple unpacking
> in Python) and list/array comprehensions.
> 
> https://developer.mozilla.org/en/New_in_JavaScript_1.7#Destructuring_assignment
> https://developer.mozilla.org/en/New_in_JavaScript_1.7#Array_comprehensions
> 
> These types of expressions make code really compact, but I'll see what I can do
> about the more explicit style guide you follow

It's not the use of destruction assignment that I'm objecting to; we use that a quite a bit for multiple returns. What I don't like is using an array/tuple as a structure. It's compact, but also prone to confusing errors where you get the order wrong. I tend to avoid that in Python too. (Or more precisely, whenever I do that in Python, I decide after a bit I should just have used a custom class to start with.)

> When I get home I'll work on making a better patch, with not so much float
> abuse. (I did have some trouble with that while testing it, so I think the
> _zoomLevel approach should be nicer. I may even do a 0-100 scale)
> 
> Couple of other questions:
> 
>  - Do I *need* to reparent the actors? I did that before I found out about
> transformedPosition and transformedSize.

I think you could get the same effect by just restacking things in all the containers so that the window and the overlay (and their ancestors) were above all siblings. Not sure if that's actually better to be worth rewriting now that you have the reparenting working.

>  - How should we best zoom in if we can't zoom in to the center of the screen?

The window should be scaled around its original center and then after scaling "pushed in" to clamp it to the screen bounds.

> I saw the problem with the mouse cursor, but I think the best approach would be
> to attach a scroll-event handler to the overlay in _zoomStart.

As noted below, I'd still like leave to end the zooming - my objection was actually more just about the visual appearance - it looked weird to have the window zoom away toward the center of the screen.

>  - I removed the zoom end on mouse because if you zoomed in a small window you
> couldn't zoom, in further because your mouse moving would cause it to leave
> scroll view. Maybe there should be an enter handler that sets a leave handler
> if there wasn't one already, but it gets reset on every click of the scroll
> wheel. Would that be a good solution?

I think if you just fix the zooming algorithm so that the zoomed window always completely covers the original window position then it will be OK.

>  - I like the overlay not covering the dash because you can mouse over
> Activities and gets out of that mode.

If the leave-the-window thing works out, this shouldn't be necesssary.

Created attachment 141596 [details] [review]
Adds zooming by scrolling (approach 3).

OK, tried this out - it's definitely getting better and better . A few more UI suggestions

 - The title bubble doesn't seem to add anything to me and I find it pretty distracting to have it sliding around - when the user chose to zoom in on the window they implicitly are saying that the title bubble wasn't useful enough (and if they zoom all the way in they can read the original title anyways.) I liked it better when I tried commenting out that part of your patch..

 - It's not obvious to me that we gain anything by tiing the fade level to the zoom level. Especially now that we are fading the top panel, continuously changing the fade level just draws the eye to things that aren't important. I tried it a) not fading at all and going immediatley to an overlay color of 0x00000044 b) using a short tween (0.15s) to that same color. Both worked OK, I think I preferred b) a little more though because it was smoother and less distracting. There was some feeling of "lag" as the window zoomed first and then the background faded, but that's primarily when you are looking at the background and not the window.

With those two adjustments it's really nice for me - a feature that I look forward to having once it lands. I'm not completely sure about the leave-zoom-mode on leave-window behavior trying it out - one alternative behavior that might be even better is:

 - When you leave the window you stay in zoom mode but the window shrinks back
 - Mousing over another window immediately enlarges that window to the current zoom level
 - When in zoom mode, clicking on a zoomed window activates it. Clicking anywhere else leaves zoom mode

(So zoom mode would let you browse between windows.)

But let's leave that as a future enhancement and get this patch in with the simpler "leave zoom on window leave" behavior.

Comments on patch:

+const PANEL_HEIGHT = Panel.PANEL_HEIGHT;

You should use Panel.PANEL_HEIGHT instead (you'll need to add the import)

+function interpolate(start, end, step) {
+    return start + (end - start) * (step / 100);
+}

Should be _interpolate instead because it's a utility. As a generic interpolation utility I think it should take 0-1, so what I'd recommend is making it:

 function interpolate(start, end, pos)

And doing:
    [actor.x, actor.y]             = this._globalOrig.interpPosition(this._target, this._step/100.);

(There's only 2-3 places where you'd need to do that /100.)

+function ScaledPoint(x, y, scaleX, scaleY) {
+    [this.x, this.y, this.scaleX, this.scaleY] = arguments;
+}

Maybe a bit overengineered, but I guess I asked for it, so I can't complain :-) Does need a brief comment explaining what it is used for.

+        if (this._step) this._zoomEnd(this.actor);

Two lines.

The variables:

 this._step, this._overlay, etc.

SHould be this._zoomStep, this._zoomOverlay, etc, since they relate to a specific portion of the Window code and not to everything.

+        function clamp(value, min, max) {
+            return Math.max(min, Math.min(max, value));
+        }

Since this is not using any local variables, I'd just put it as _clamp along with _interpolate at the top scope.

+    _zoomStart : function (actor) {
+
+        let global = Shell.Global.get();

No blank line at the start of a function.

+        this._target = new ScaledPoint(0, 0, 1.0, 1.0);
+        this._target.setPosition(actor.x - (actor.width - width) / 2, actor.y - (actor.height - height) / 2);

Doesn't make sense to me to create it, then initially override the 0,0 with something else.

+        this._localPosition  =
+        this._localScale     =
+        this._globalPosition =
+        this._globalScale    =
+        this._targetPosition =
+        this._step = undefined;

That draws the eye and looks like a bug for minimal savings, I'd just repeat the 'undefined'

+        if (event.get_button() == 3 && actor.step != undefined)

You don't have actor.step any more, and I still don't think the right-button has any value. :-)

Whitespace changes, shell_get_scroll_event_direction() need to be cleaned out of the patch.

Patch needs a commit message (the good thing about 'git commit --amend' is that you can write a nice message once, and then keep on improving it as you work without losing it.)

You probably want to make a general runthrough and do a cleanup of places that could use a comment or two (heavy commenting is definitely not necessary, the code can talk for itself, but explaining things that aren't locally obvious), left-over commented out code, etc.

Created attachment 142444 [details] [review]
Adds zooming by scrolling (approach 4).

New patch.

OK, comparing 3 and 4:

(In reply to comment #9)
>  - The title bubble doesn't seem to add anything to me

agree. 4 is better here.

>  - It's not obvious to me that we gain anything by tiing the fade level to the
> zoom level. Especially now that we are fading the top panel, continuously
> changing the fade level just draws the eye to things that aren't important. I
> tried it a) not fading at all and going immediatley to an overlay color of
> 0x00000044 b) using a short tween (0.15s) to that same color. Both worked OK, I
> think I preferred b) a little more though because it was smoother and less
> distracting.

I don't see the need for the fade anyway, and found it distracting, like you said. If we're going to keep it, it definitely needs a tween.

The fading is especially confusing when "zooming" a window that is already at 100% even in the overview. (Eg, if you run "gnome-shell --xephyr" and immediately enter the overview, the xlogo and xeyes windows will be at 100%.) They don't change size, but everything else still darkens around them.

> There was some feeling of "lag" as the window zoomed first and
> then the background faded, but that's primarily when you are looking at the
> background and not the window.

Well, the zooming/unzooming needs to tween too. The current approach of jumping directly from one size to another is un-GNOME-Shell-like.

> I'm not completely sure about the
> leave-zoom-mode on leave-window behavior trying it out - one alternative
> behavior that might be even better is:
> 
>  - When you leave the window you stay in zoom mode but the window shrinks back
>  - Mousing over another window immediately enlarges that window to the current
> zoom level
>  - When in zoom mode, clicking on a zoomed window activates it. Clicking
> anywhere else leaves zoom mode
> 
> (So zoom mode would let you browse between windows.)

version 4 implements that, but I don't like it. The problem may just be that the dark overlay disappears when you move between windows, so it looks like you've left "zoom mode" even though you haven't.

But if people really need to have a persistent zooming mode to be able to use the overview effectively, then it seems like zooming should just happen automatically on mouseover all the time.

> >  - It's not obvious to me that we gain anything by tiing the fade level to the
> > zoom level. Especially now that we are fading the top panel, continuously
> > changing the fade level just draws the eye to things that aren't important. I
> > tried it a) not fading at all and going immediatley to an overlay color of
> > 0x00000044 b) using a short tween (0.15s) to that same color. Both worked OK, I
> > think I preferred b) a little more though because it was smoother and less
> > distracting.
>
> I don't see the need for the fade anyway, and found it distracting, like you
> said. If we're going to keep it, it definitely needs a tween.
> 
> The fading is especially confusing when "zooming" a window that is already at
> 100% even in the overview. (Eg, if you run "gnome-shell --xephyr" and
> immediately enter the overview, the xlogo and xeyes windows will be at 100%.)
> They don't change size, but everything else still darkens around them.

Well, then the fade/light-boxing gives you feedback that you did the "zoom" thing and there was nothing to soon...

I thought the fade worked well when it wasn't so dark (as mentioned above) and there was a quick tween - in this patch appearning abruptly 0x000000AA it's a bit too much.
 
> > There was some feeling of "lag" as the window zoomed first and
> > then the background faded, but that's primarily when you are looking at the
> > background and not the window.
> 
> Well, the zooming/unzooming needs to tween too. The current approach of jumping directly from one size to another is un-GNOME-Shell-like.

The problem is that there's a natural rate at which you expect it to zoom, which is the rate you are turning the scroll wheel. If it animates faster then that, you'll get a zoom-stop zoom-stop zoom-stop behavior. If you animate slower than that, then it will seem laggy.

But when we get the first scroll event we have no idea what that natural rate is. (Eventual support for continuous mouse-wheels in the X stack would help).

Maybe we could figure out something that would look OK - some combination of easing effects and intervals that disguised our lack of knowledge, but it's going to be tricky.

> > I'm not completely sure about the
> > leave-zoom-mode on leave-window behavior trying it out - one alternative
> > behavior that might be even better is:
> > 
> >  - When you leave the window you stay in zoom mode but the window shrinks back
> >  - Mousing over another window immediately enlarges that window to the current
> > zoom level
> >  - When in zoom mode, clicking on a zoomed window activates it. Clicking
> > anywhere else leaves zoom mode
> > 
> > (So zoom mode would let you browse between windows.)
> 
> version 4 implements that, but I don't like it. The problem may just be that
> the dark overlay disappears when you move between windows, so it looks like
> you've left "zoom mode" even though you haven't.
> 
> But if people really need to have a persistent zooming mode to be able to use
> the overview effectively, then it seems like zooming should just happen
> automatically on mouseover all the time.

Well, the reaosn that you are zooming in is you can't distinguish between some identical windows, or you want to see more detail. But that doesn't imply that that's *always* the case.

I don't think the browsing mode is quite ready, however, either from an implementation or UI experience. It's a bit flashy as you move between windows, and I don't like that you can be in the browsing mode but not see it

Can we try to land a patch without that and treat that as a separate feature on top?

Generally, the patch looks nicely cleaned up. Just a few things I still see:

+    return start + (end - start) * (step);

extra parens.

+    return start + (end - start) * (step);
+}
+
+
+function _clamp(value, min, max) {

extra newline.

+        [actor.x, actor.y]             = this._globalOrig.interpPosition(this._target, this._zoomStep / 100);
+        [actor.scale_x, actor.scale_y] = this._globalOrig.interpScale(this._target, this._zoomStep / 100);
+
+        let global = Shell.Global.get();

Our style is always to assign global at the top of the function. (The idea is that if we get more things in the function that use 'global', we don't want to have to move it.)

+    _zoomUpdate : function (actor) {

zoomStart/zoomUpdate should not take the actor parameter, since it is always just this.actor.

+        this._hideEventId = Main.overview.connect('hidden', Lang.bind(this, function (a, e) { this._zoomEnd(this.actor); }));

You want to connect to 'hiding' not hidden, so it happens at the start of leaving the overlay, not the end.

THe callback for the signal should just function() not function(a,e) (there's no '[a]ctor' or '[e]event' here.

>> The variables:
>>
>> this._step, this._overlay, etc.
                              ^^^
>> Should be this._zoomStep, this._zoomOverlay, etc, since they relate to a
>> specific portion of the Window code and not to everything.

By "etc" I meant all the member variables you were adding to window.

+        this._zoomOverlay = props.overlay || new Clutter.Rectangle({ reactive: true,
+                                                                     color: OVERLAY_COL
+                                                                     border_width: 0,
+                                                                     x: 0, y: 0,
+                                                                     width: global.scre
+                                                                     height: global.scr
+                                                                     opacity: 255 });

The need for this should go away with the temporary removal of browse mode, but if it wasn't, in our style this would just be an 'if'. (Using short-circuiting || like this is confusing and error-prone. Using the ternary ?: is OK, but is going to be awkward given the length of the call to new Clutter.Rectangle.)

Can you do a new version of the patch:

 - Without the browse mode
 - With the changes noted above
 - In 'git format-patch' form with a commit message

And then hopefully we can land that patch and move forward?

(In reply to comment #12)
> > Well, the zooming/unzooming needs to tween too. The current approach of jumping directly from one size to another is un-GNOME-Shell-like.
> 
> The problem is that there's a natural rate at which you expect it to zoom,
> which is the rate you are turning the scroll wheel.

Right. But the animation doesn't need to think about speed-of-scrolling. You just decide what size the window should be based on how far the user has scrolled, and tween to that size in a fixed amount of time T. If the user is scrolling faster than 1 click per T seconds, then the initial tween to base_scale*1.2 will get cut off and replaced by a tween to base_scale*1.4, which means it has to start animating faster to reach that scale in the next T seconds, and then if you scroll a notch further it will have to animate even faster, etc, but it will always catch up to you T seconds after you stop scrolling.

Created attachment 142494 [details] [review]
tween the zooming

proof of concept, seems to work ok. (linear looks at little better than
easeOutQuad in the final zoom steps of large windows, imho)

Created attachment 142723 [details] [review]
Scroll wheel should zoom windows in the overview

Updated patch addressing Owen's comments. This tweens the overlay fade-in
(although I don't think the fade is very noticeable at 0.15s), but not the
zooming.

Review of attachment 142723 [details] [review]:

The background tween isn't very noticable - but that's exactly the point - the whole reason to add the tween is to make it *less* noticable that the tween is happening.

One comment below, otherwise looks good to commit. Why don't you go ahead and do that, Dan, so we can do other changes that touch these code portions without creating conflicts.

::: js/ui/workspaces.js
@@ -28,0 +31,3 @@
+const ZOOM_OVERLAY_FADE_TIME = 0.15;
+const ZOOM_OVERLAY_COLOR = new Clutter.Color();
+ZOOM_OVERLAY_COLOR.from_pixel(0x000000AA);

It really looks better to me if this is 0x00000044. (Which makes it the same as LIGHTBOX_COLOR which is above since the menu landed ... maybe just use that.)

committed (with the LIGHTBOX_COLOR change)