chaco

chaco copied to clipboard

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.

Can one of the admins verify this patch?

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

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

Thanks!

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.

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.

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

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

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

Thanks,

Another try...!

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

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.

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

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

@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.