Created attachment 173569 [details] [review]
Refactor Magnifier.ZoomRegion to avoid permanent Clutter.Clone

Assigning for review; I've tried to test this as thoroughly as possible and all the settings options seem to work and properly interact.

Created attachment 173571 [details] [review]
Refactor Magnifier.ZoomRegion to avoid permanent Clutter.Clone

Tiny fix to make the mouse pointer move correctly when in NONE mouse
tracking mode.

Created attachment 173572 [details] [review]
Refactor Magnifier.ZoomRegion to avoid permanent Clutter.Clone

The actual fixed-up version I meant to attach, without random cruft
included.

Review of attachment 173572 [details] [review]:

Nice refactoring, but there are a few functional issues.  I've noted in code what the fixes are, but, briefly, here are the three main concerns:

1. Starting in a state with the magnifier on, and with crosshairs on:  turning the magnifier off, then back on results in the crosshairs vanishing, and they will not reappear.

2. If there is more than one zoom region, the magnified mouse in the other zoom regions is stuck at the top left corner, and does not move.

3. If the region of interest does not contain the mouse, and the magnification factor is changed, the magnified view incorrectly "warps" to the position of the mouse.

::: js/ui/magnifier.js
@@ +217,3 @@
+
+        // We ignore the redundant width/height on the ROI
+        let fixedROI = new Object(roi);

I don't see the purpose of passing <roi> through the Object constructor.  Since roi isa Object, the object returned is identical, that is fixedROI === roi.

I'm not saying it's wrong, but I don't see what it buys you.

@@ +261,2 @@
         }
         this._zoomRegions[0].setActive(false);

Given that the first ZoomRegion is no longer "cached", this loop should run from zero to _zoomRegions.length.  And, the trailing setActive() just after the loop can be deleted.

@@ +267,2 @@
         if (this._crossHairs)
             this._crossHairs.removeFromParent();

This is duplicated now in ZoomRegion._destroyActors().  It is here since the Magnifier object allocated the Crosshairs object and manages it. Since this is where all of the zoom regions are eliminated, it's the place to deal with the final decoupling of the cross hairs tree from its parent.

Still, I'm inclined to remove it from here, and let the ZoomRegion request the decoupling in its _destroyActors(); but, it should do that in consultation with the Crosshairs object itself.  See my comments in ZoomRegion._destroyActors() below.

@@ +588,3 @@
+        this._uiGroupClone = null;
+        this._mouseRoot = mouseRoot;
+        this._mouseRootActor  = null;

I suggest "_mouseRootClone" since, for all of the ZoomRegion's except the first, it is a clone of _mouseRoot.  It helps document what is going on here.

@@ +638,3 @@
+        this._changeROI({ xMagFactor: xMagFactor,
+                          yMagFactor: yMagFactor,
+                          redoCursorTracking: true });

The redoCursorTracking does not work in all cases.  The problem occurs when the mouse cursor is not in the ROI, but the user's regard is.  Changing the mag factor then incorrectly shifts the view to where the mouse is.

This can happen when using the magnifier in conjunction with Orca's "focus tracking".  Here, the user navigates the UI using the keyboard, and as each user interface element (e.g., a menu item) gains focus, Orca tells the magnifier to place that element at the centre of the magnified view. If the element in question is, say, a spinner for changing the mag factor, then changing the value of the spinner using the keyboard suddenly moves the ROI to the mouse; but, it's imperative that the ROI stay on the spinner.  The same thing will occur when the mag factor was controlled with a key press (and the mouse wasn't within the ROI).

That's why I chose the re-centre approach -- keep the centre of the ROI at the centre of the view as mag factor changes.

@@ +685,3 @@
      * Sets the "region of interest" that the ZoomRegion is magnifying.
      * @roi:    Object that defines the region of the screen to magnify.  It
+     *          has has members x, y, width, height.  The values are in

Nit:  two "has"'s.

@@ +890,1 @@
      * @crossHairs  Clutter.Group that contains the actors for the crosshairs.

My bad.  Although at one point, this was a Clutter.Group, it ended up being a Crosshairs object, that, in turn, has the Clutter.Group as a private data member.  This results in an issue in ZoomRegion._destroyActors().  See the comments there.

@@ +941,3 @@
+            this._mouseRootActor.get_parent().remove_actor (this._mouseRootActor);
+        if (this._crossHairs && this._crossHairsActor == this._crossHairs)
+            this._crossHairsActor.get_parent().remove_actor (this._crossHairsActor);

This won't work since _crossHairsActor is *not* a clone of this._crossHairs.  In fact, _crossHairs isa Crosshairs object and _crossHairsActor is either the private actor data member within _crossHairs, or a clone of that private data member.

That is:
_crossHairsActor === _crossHairs._actor
Or:
_crossHairsActor === a clone of _crossHairs._actor

There should be a method "Crosshairs.removeFromZoomRegion()" that handles this.  It would be symmetrical with the current Crosshairs.addToZoomRegion().

@@ +951,3 @@
+
+    _setViewPort: function(viewPort, fromROIUpdate) {
+        // Sets the position of the magnifier on the screen

Nit:  Sets the position of the zoom region on the screen.

@@ +989,3 @@
+            params.xMagFactor = 1;
+        if (params.yMagFactor <= 0)
+            params.yMagFactor = 1;

There are users who want minification.  Other magnifiers support minification, for example, FireFox's zoom function within its "View" menu.

I "clamped" mag factor to greater-than-zero for that purpose, leaving the current value untouched if the passed in value was zero or less.

@@ +1158,1 @@
         this._mouseRoot.set_position(xMagMouse, yMagMouse);

This should be "_mouseRootActor":

this._mouseRootActor.set_position(xMagMouse, yMagMouse);

Or "_mouseRootClone" if you agree with the name change I suggested above.

If the original "_mouseRoot" is used, then for zoom regions other than the first, the magnified mouse is not in the correct location at all.

It works in the current code because ZoomRegion has a single "_mouseRoot" data member initialized to either the original or the clone as appropriate.  The patch uses two data members to track which is which.  The relevant data member to reposition the magnified mouse is, then, "_mouseRootActor".  This will work for all ZoomRegion's.

::: js/ui/magnifierDBus.js
@@ +116,3 @@
+     * FIXME: The arguments here are redundant, since the width and height of
+     *   the ROI are determined by the viewport and magnification factors.
+     *   We ignore the passed in width and height.

Yes, the width and height appear irrelevant.  If the ROI is larger than the viewport, then it is clipped.  If smaller, then the user sees more.  Neither appears to be a bug.  All that is required is a known point such as the top left or centre of the ROI.

There is a discussion regarding a common magnifier API:
http://live.gnome.org/Accessibility/MagnificationFramework

I'll bring the issue up with the gnome-mag developers, and others, and see what they say.

(In reply to comment #5)
> Review of attachment 173572 [details] [review]:
> 
> Nice refactoring, but there are a few functional issues.  I've noted in code
> what the fixes are, but, briefly, here are the three main concerns:
> 
> 1. Starting in a state with the magnifier on, and with crosshairs on:  turning
> the magnifier off, then back on results in the crosshairs vanishing, and they
> will not reappear.
> 
> 2. If there is more than one zoom region, the magnified mouse in the other zoom
> regions is stuck at the top left corner, and does not move.
> 
> 3. If the region of interest does not contain the mouse, and the magnification
> factor is changed, the magnified view incorrectly "warps" to the position of
> the mouse.

Either way is sort of bad. What I was trying to correct was a problem observed in testing where if you change the zoom region the view would change and then if you moved the mouse the view would jump around again. Maybe what we can do is record if the last ROI change was from a mouse motion or not. If the ROI is tracking the mouse it should stay glued to the mouse. Will attach a new version with that change.

(I think long term we may want to put the following-the-caret smarts into gnome-shell and not depend on something like Orca do do that .... couple of reasons - first it's useful for all users that have the magnifier on, and second that would allow better smarts about when we should shift the ROI. But that's not at all immediately.)

> ::: js/ui/magnifier.js
> @@ +217,3 @@
> +
> +        // We ignore the redundant width/height on the ROI
> +        let fixedROI = new Object(roi);
> 
> I don't see the purpose of passing <roi> through the Object constructor.  Since
> roi isa Object, the object returned is identical, that is fixedROI === roi.
> 
> I'm not saying it's wrong, but I don't see what it buys you.

It would make more sense if I actually used fixedROI after creating it. :-) The point is that I can't modify the 'roi' object that was passed in by the caller because it's the property of the caller, so I have to make a copy to fix it up to pass on to my callee.

> @@ +261,2 @@
>          }
>          this._zoomRegions[0].setActive(false);
> 
> Given that the first ZoomRegion is no longer "cached", this loop should run
> from zero to _zoomRegions.length.  And, the trailing setActive() just after the
> loop can be deleted.

Yeah, can be simplified.

> @@ +267,2 @@
>          if (this._crossHairs)
>              this._crossHairs.removeFromParent();
> 
> This is duplicated now in ZoomRegion._destroyActors().  It is here since the
> Magnifier object allocated the Crosshairs object and manages it. Since this is
> where all of the zoom regions are eliminated, it's the place to deal with the
> final decoupling of the cross hairs tree from its parent.
> 
> Still, I'm inclined to remove it from here, and let the ZoomRegion request the
> decoupling in its _destroyActors(); but, it should do that in consultation with
> the Crosshairs object itself.  See my comments in ZoomRegion._destroyActors()
> below.

Agree should be in destroyActors.

> @@ +588,3 @@
> +        this._uiGroupClone = null;
> +        this._mouseRoot = mouseRoot;
> +        this._mouseRootActor  = null;
> 
> I suggest "_mouseRootClone" since, for all of the ZoomRegion's except the
> first, it is a clone of _mouseRoot.  It helps document what is going on here.

I've renamed:

 mouseRoot => mouseSourceActor
 mouseRootActor => mouseActor

Which I think is clear and makes something like mouseSourceActor.set_position() jump out as wrong.

> @@ +638,3 @@
> +        this._changeROI({ xMagFactor: xMagFactor,
> +                          yMagFactor: yMagFactor,
> +                          redoCursorTracking: true });
> 
> The redoCursorTracking does not work in all cases.  The problem occurs when the
> mouse cursor is not in the ROI, but the user's regard is.  Changing the mag
> factor then incorrectly shifts the view to where the mouse is.
> 
> This can happen when using the magnifier in conjunction with Orca's "focus
> tracking".  Here, the user navigates the UI using the keyboard, and as each
> user interface element (e.g., a menu item) gains focus, Orca tells the
> magnifier to place that element at the centre of the magnified view. If the
> element in question is, say, a spinner for changing the mag factor, then
> changing the value of the spinner using the keyboard suddenly moves the ROI to
> the mouse; but, it's imperative that the ROI stay on the spinner.  The same
> thing will occur when the mag factor was controlled with a key press (and the
> mouse wasn't within the ROI).
> 
> That's why I chose the re-centre approach -- keep the centre of the ROI at the
> centre of the view as mag factor changes.

See above.

> @@ +685,3 @@
>       * Sets the "region of interest" that the ZoomRegion is magnifying.
>       * @roi:    Object that defines the region of the screen to magnify.  It
> +     *          has has members x, y, width, height.  The values are in
> 
> Nit:  two "has"'s.
> 
> @@ +890,1 @@
>       * @crossHairs  Clutter.Group that contains the actors for the crosshairs.
> 
> My bad.  Although at one point, this was a Clutter.Group, it ended up being a
> Crosshairs object, that, in turn, has the Clutter.Group as a private data
> member.  This results in an issue in ZoomRegion._destroyActors().  See the
> comments there.
> 
> @@ +941,3 @@
> +            this._mouseRootActor.get_parent().remove_actor
> (this._mouseRootActor);
> +        if (this._crossHairs && this._crossHairsActor == this._crossHairs)
> +            this._crossHairsActor.get_parent().remove_actor
> (this._crossHairsActor);
> 
> This won't work since _crossHairsActor is *not* a clone of this._crossHairs. 
> In fact, _crossHairs isa Crosshairs object and _crossHairsActor is either the
> private actor data member within _crossHairs, or a clone of that private data
> member.
> 
> That is:
> _crossHairsActor === _crossHairs._actor
> Or:
> _crossHairsActor === a clone of _crossHairs._actor
> 
> There should be a method "Crosshairs.removeFromZoomRegion()" that handles this.
>  It would be symmetrical with the current Crosshairs.addToZoomRegion().

Ah. Well, we have removeFromParent() right? I've now adapted that to take a 'childActor' parameter.

[ I think addtoZoomRegion() is probably not the right way to go about it. Actors and actor-like objects really shoudn't have knowledge about what they will be added to and how they behave in them. Also, I'm of the opinion that using clones for the pointer and the cross-hairs is likely more trouble than it's worth - clones are generally speaking *less* efficient than multiple actors. But not going to try to change anything about that here. ]

> @@ +951,3 @@
> +
> +    _setViewPort: function(viewPort, fromROIUpdate) {
> +        // Sets the position of the magnifier on the screen
> 
> Nit:  Sets the position of the zoom region on the screen.

Fixed.
 
> @@ +989,3 @@
> +            params.xMagFactor = 1;
> +        if (params.yMagFactor <= 0)
> +            params.yMagFactor = 1;
> 
> There are users who want minification.  Other magnifiers support minification,
> for example, FireFox's zoom function within its "View" menu.
> 
> I "clamped" mag factor to greater-than-zero for that purpose, leaving the
> current value untouched if the passed in value was zero or less.

Note the above is that values <= 0 are treated as a default of 1. Nothing to do with minification. It could be:

        if (params.xMagFactor <= 0)
            params.xMagFactor = this._xMagFactor;

as easily. Don't think it makes any difference but I've changed it that way.

> @@ +1158,1 @@
>          this._mouseRoot.set_position(xMagMouse, yMagMouse);
> 
> This should be "_mouseRootActor":
> 
> this._mouseRootActor.set_position(xMagMouse, yMagMouse);
> 
> Or "_mouseRootClone" if you agree with the name change I suggested above.
> 
> If the original "_mouseRoot" is used, then for zoom regions other than the
> first, the magnified mouse is not in the correct location at all.
> 
> It works in the current code because ZoomRegion has a single "_mouseRoot" data
> member initialized to either the original or the clone as appropriate.  The
> patch uses two data members to track which is which.  The relevant data member
> to reposition the magnified mouse is, then, "_mouseRootActor".  This will work
> for all ZoomRegion's.

OK, if you could test my new patch when I attach it I'd appreciate it. I don't really have a way of testing multiple zoom regions, though I suppose I could figure something out with the d-feet DBus debugger.

> ::: js/ui/magnifierDBus.js
> @@ +116,3 @@
> +     * FIXME: The arguments here are redundant, since the width and height of
> +     *   the ROI are determined by the viewport and magnification factors.
> +     *   We ignore the passed in width and height.
> 
> Yes, the width and height appear irrelevant.  If the ROI is larger than the
> viewport, then it is clipped.  If smaller, then the user sees more.  Neither
> appears to be a bug.  All that is required is a known point such as the top
> left or centre of the ROI.

You could, yes, declare that the ROI passed in when creating the zoom region is just a funny way to pass a point. Would be better not to have duplication, though.
 
> There is a discussion regarding a common magnifier API:
> http://live.gnome.org/Accessibility/MagnificationFramework
> 
> I'll bring the issue up with the gnome-mag developers, and others, and see what
> they say.

Many thanks for the detailed review and for finding the bugs in my patch!

Created attachment 175493 [details] [review]
Fix up magnification refactoring

[ To squash with refactoring ]

* Use fixedROI after setting it
* Remove unused code for setting the magnifier inactive
* Rename ZoomRegion._mouseRoot => ZoomRegion._mouseSourceActor
* Keep track of ZoomRegion._followingCursor to know when we should
  redo the mouse tracking on viewport/magnificiation changes
* Fix removing the crosshair when destroying the actors
* Correctly update mouse position for secondary zoom regions

(In reply to comment #6)
> > 3. If the region of interest does not contain the mouse, and the magnification
> > factor is changed, the magnified view incorrectly "warps" to the position of
> > the mouse.
> ...
> (I think long term we may want to put the following-the-caret smarts into
> gnome-shell and not depend on something like Orca do do that .... couple of
> reasons - first it's useful for all users that have the magnifier on, and
> second that would allow better smarts about when we should shift the ROI. But
> that's not at all immediately.)

Agreed, and I'll go one better.  A number of assistive technologies need to know where keyboard focus is including, but not limited to, screen readers, magnifiers, and onscreen keyboards.  It would be very useful if there was a focus tracking module that any app could use.

> > ... There are users who want minification.  Other magnifiers support minification...
> > 
> ... Nothing to do
> with minification.

D'oh!

> 
> OK, if you could test my new patch when I attach it I'd appreciate it. I don't
> really have a way of testing multiple zoom regions, though I suppose I could
> figure something out with the d-feet DBus debugger.

I use this (see url below), although it's rough and currently my copy is out of sync with what's in the repository.  I'll be merging the two some time this week.
http://git.gnome.org/browse/gnome-mag/tree/test/dbus-mag-test.py?h=bonobo-less

> 
> Many thanks for the detailed review and for finding the bugs in my patch!

You're welcome!  Next review coming up.

Review of attachment 175493 [details] [review]:

Except for some minor book keeping, this looks good.  I've put it through my testing procedure, and everything works as expected.

::: js/ui/magnifier.js
@@ +1240,3 @@
+            childActor.get_parent().remove_actor(childActor);
+        else
+            childActor.destroy();

The Crosshairs object maintains an array of the clones.  Before destroying the cloned actor, that array should be updated:

else {
    let idx = this._clones.indexOf(childActor);
    if (idx != -1) this._clones.splice(idx, 1);
    childActor.destroy();
}

Attachment 173572 [details] pushed as c5c66ce - Refactor Magnifier.ZoomRegion to avoid permanent Clutter.Clone