Fixing Bad behavior of AspectRatio for ZoomTool by updating better_selecting_zoom.py

[SDiot]

Hello,

This follows issue #191 that I posted a month ago.
This is just a copy and paste of #191 but this time as a pull request.
This was also the topic of the pull request #132 but it has not been done and closed.
I'm thus trying to do it again so that it's done since I need it for a project.

I don't know what are the other steps required to ensure the pull-request will be done, so I'm looking forward for your feedback!

Thanks in advance,

S.D.

---

PROBLEM and FIX:

I was trying to use the aspect_ratio keyword of ZoomTool and was expecting:

1. the blue overlay that show the box zoom to be forced to respect the aspect_ratio
2. the final zoomed view to correspond to the blue overlay

Unfortunately, while 1) is indeed the case, 2) is not. The zoom is actually done according to the mouse position at the time of unclicking. I think that is a bug since it implies that the aspect_ratio is not respected.

Looking at the file better_selecting_zoom.py, this is due to self._screen_end being set to (event.x, event.y) when entering _end_select on line 330. Because of that the appropriate value of self._screen_end that is set in selecting_mouse_move according to the aspect_ratio keyword is forgotten.

After commenting line 330 of file better_selecting_zoom.py, I obtain the exact behavior I was expecting.

A simple example is given here: https://github.com/SDiot/chaco_troubles/blob/master/ZoomTool_AspectRatio.py (Just have to follow the steps given in the file doc string!)

I think that this is a bug and should be fixed. If it's not, please let me know why! :)

Thanks!

S.D.

[enbuilder]

Can one of the admins verify this patch?

[coveralls]

Coverage increased (+0.0%) when pulling d663bd2 on SDiot:master into c5af188 on enthought:master.

[SDiot]

Thanks to the fix of pberkes in #197, it should now work.
Could someone start the Travic CI build over??

Thanks!

[jonathanrocher]

Hey Steven, you are going to need to merge current master into your branch so that @pberkes 's fix get applied to your PR as well. I just relaunched a build that failed for the same reason. Ping me when done.

[SDiot]

Hi Jonathan,
Sorry for the delay, I had to find time to understand GitHub!
Seems like things are now good, right? What's the next step?

Thanks for the help,

S.

[coveralls]

Coverage increased (+0.0%) when pulling 8911a22 on SDiot:master into 98d9579 on enthought:master.

[SDiot]

Could someone merge the fix to the enthought:master so that we canc lose this PR?
Thanks,

[SDiot]

Up! In case someone has time to merge it! :)

Thanks,

[SDiot]

Another try...!

[coveralls]

Coverage increased (+10.95%) when pulling 8171fb8 on SDiot:master into 98d9579 on enthought:master.

[SDiot]

I just added a modification of polygon_plot.py that I'll PR soon, but I didn't think it would go into this PR... Could you only merge the fix for ZoomTool without merging polygon_plot.py or should I remove it for now?

Sorry about that.

[SDiot]

Back to original polygon_plot.py to avoid confusion. I'll create another fork for the list of polygons thing...

[coveralls]

Coverage increased (+10.98%) when pulling 9e187ad on SDiot:master into 98d9579 on enthought:master.

[jonathanrocher]

@cfarrow @rkern @corranwebster @... Could you have a look to this (1 line chance)? It seems to me like @SDiot is right and the behavior in this branch is what you expect.