Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added new selection ‘Move’ mode #689

Merged
merged 4 commits into from
Jul 6, 2015

Conversation

astrofrog
Copy link
Member

Fixes #688 by adding a dedicated 'Move' mode for selections which is not enabled by default.

@astrofrog
Copy link
Member Author

One issue I'm having is that if I click outside a ROI, then everything works fine but there is a small delay because it re-computes the subset anyway.

@ChrisBeaumont - do you have any suggestions for how to short-circuit things in the case where I click outside the region when in move mode? (if not, I'll dig into the code)

@astrofrog astrofrog added this to the 0.5.1 milestone Jul 3, 2015
@ChrisBeaumont
Copy link
Member

The move mode doesn't feel like the other edit subset modes for me for a
few reasons:

  • the logic of updating the subset is the same as the replace mode, and
    not a distinct operation
  • the other subset modes define what happens when a new subset state
    should be applied. This mode defines details about how mouse behavior works
  • one of the design considerations for subset modes was that they are a
    black box as far as the rest of the code base is concerned. This mode
    breaks that contract, since this or adds several new branches to the code
    base checking the mode state

If the intention is to toggle between scrubbing and redefining subsets,
perhaps this should be controlled by another mechanism. For example,
holding a modifier key like shift while dragging. Or maybe making it so
backspace just clears the current subset

On Friday, July 3, 2015, Thomas Robitaille notifications@github.com wrote:

One issue I'm having is that if I click outside a ROI, then everything
works fine but there is a small delay because it re-computes the subset
anyway.

@ChrisBeaumont https://github.com/ChrisBeaumont - do you have any
suggestions for how to short-circuit things in the case where I click
outside the region when in move mode? (if not, I'll dig into the code)


Reply to this email directly or view it on GitHub
#689 (comment).

@astrofrog
Copy link
Member Author

@ChrisBeaumont - thanks for reviewing this and for the insight. I agree that this is a bit different to a mode. I like the suggestion of having a modifier key for switching to scrubbing as opposed to backspace for clearing selections. I'll look into it.

@astrofrog
Copy link
Member Author

@ChrisBeaumont - thanks for your suggestion, I think the code is nicer now. I decided to use the control key for now, since shift, option, and command already have common meanings with selections that we may want to implement in glue some day (i.e. what we were doing in bermuda). Would you be happy with this implementation?

@astrofrog
Copy link
Member Author

@ChrisBeaumont - just a small question: why does the circular selection appear to work in pixel coordinates while others appear to work in data coordinates?

@ChrisBeaumont
Copy link
Member

They should probably all work in pixel coordinates, because a circle on the
screen is not a circle in data space for, eg, projections and log
transforms.

On Sunday, July 5, 2015, Thomas Robitaille notifications@github.com wrote:

@ChrisBeaumont https://github.com/ChrisBeaumont - just a small
question: why does the circular selection appear to work in pixel
coordinates while others appear to work in data coordinates?


Reply to this email directly or view it on GitHub
#689 (comment).

…ubbing would stop until user went over region again.
@astrofrog
Copy link
Member Author

@ChrisBeaumont - ok I'll open another issue for that. I'll try and find some examples where the effects would be noticeable to try and fix this.

@astrofrog
Copy link
Member Author

@ChrisBeaumont - ok, thanks! This PR now works nicely (there was a subtle bug I introdued that was causing what looked like slowness/lagging in the scrubbing) Once this passes, I'll merge it later this evening or tomorrow if there are no objections.

astrofrog added a commit that referenced this pull request Jul 6, 2015
Added new selection ‘Move’ mode
@astrofrog astrofrog merged commit cf08470 into glue-viz:master Jul 6, 2015
@stscieisenhamer
Copy link
Contributor

Only because somewhere I was mentioned, but I was on vacation and then dealing with the resulting work-catchup, but wanted to drop...

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants