Drag other selections

[stscieisenhamer]

This allows the dragging of the Circular and Polygon (lasso) selections, as with Rectangle, resolving #651

Note also this has the fix in PR #641 by accident. I can remove if desired.

[ChrisBeaumont]

Probably best to remove #641 to keep the reviews straight

[ChrisBeaumont]

So the following scenario is annoying at the moment:

• Open an image, define a circle that fills the page
• Try to drag big circle out of way to define a new one, cannot get it to move
• Click over to rectangle ROI, define a small rectangle
• Click back to circle ROI, find that my mega circle was saved, and is still there

A few possible ways to address this:

• If I click on the edge of a circle and drag, the circle immediately snaps so that the center is under my mouse. I'd expect the same part of the circle to stay under the mouse (and this makes it impossible to drag big circles off the page)
• Maybe, all shapes should clear on escape key press, like the lasso ROI
• ROIs should probably be deleted when switching mouse modes, so that clicking away and back doesn't resurrect the stale ROI

thoughts?

[ChrisBeaumont]

Ah I see you prefer to bundle #641 into this PR. That's fine

[ChrisBeaumont]

these changes look fine to me

[stscieisenhamer]

Ewww, yes, the circle should behave has the other shapes. Also agree on the global use of the escape key. And will look into the stale ROI issue.

• Set circle grab point where cursor lies.
• Add reset on key event(escape) to non-vertex based ROI's
• Reset ROI state on change of ROI shape and non-ROI events

[astrofrog]

@stscieisenhamer - just a minor nit, but if possible, it would be great to get rid of the merge commits by doing a rebase (I think there won't be any conflicts?). In general, you can keep the history cleaner by rebasing instead of merging.

[stscieisenhamer]

Oops, sorry, will do the rebase. Just getting back to the other two points above, after which rebase will occur.

[stscieisenhamer]

OK. The three issues have been dealt with. And I think I've done the rebase, though next time I'll make sure to start with rebasing. So, should be ready for another review.

[ChrisBeaumont]

wrap these in list() to preserve python3 compatibility

[ChrisBeaumont]

Looks good to me, pending the python3 issue!

[stscieisenhamer]

Question of procedure: should I merge or is there something else that need be done?

[astrofrog]

@stscieisenhamer - it would be good to rebase to get rid of the merge commits (it doesn't look like the rebase before worked). Could you try again? Normally when you rebase against the upstream master, it should automatically remove the merge commits.

(Procedure wise, as long as @ChrisBeaumont and I have signed off and said it's fine to merge, it doesn't matter who actually merges)

[stscieisenhamer]

Think I've gotten out of the 'rebase cycle' and have successfully (?) done that. Just a question of the coverage test fail; I haven't dealt with that before and if pointed in the right direction will rectify.

[ChrisBeaumont]

@stscieisenhamer the coverage test fails when the overall unit test coverage decreases. It would be great if you can add some unit tests for the new functions you added, in glue.core.tests.test_roi.py (a coveralls failure isn't a non-starter, but in this case there's are enough state + edge cases that it's probably worth testing for)

Let me know if you need guidance running the test suite locally (see https://github.com/glue-viz/glue/blob/master/.travis.yml#L122)

[ChrisBeaumont]

Nice!