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

Persistent selections for cartesian subplots #6243

Merged
merged 105 commits into from Jul 13, 2022
Merged

Persistent selections for cartesian subplots #6243

merged 105 commits into from Jul 13, 2022

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Jun 22, 2022

Demo

Please find new layout attributes in b952125 which are quite similar to shapes.

More description: coming soon

Some TODOs:

  • revise x0, x1, y0, y1 defaults
  • handle splom
  • improve activating selections
  • improve subtract cases & multi-polygon cases
  • additional work on tests
  • consider Chris' comments on the slack channel

@plotly/plotly_js
cc: @chriddyp

@@ -377,6 +377,8 @@ function prepSelect(evt, startX, startY, dragOptions, mode) {
throttle.done(throttleID).then(function() {
throttle.clear(throttleID);

// Only points selected by the new selection are presented in eventData here
// Should we provide all the selected points instead?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think so 😄 (this is basically #4095, right?)
As it stands when you edit one of the selections later, the points from all selections are included; we should do that when you create a new selection as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this plotly_selected one returns points similar to the plotly_selecting but it fires after a new selection not during the process of selecting. The shift/alt missing part is actually addressed in b9d2db8.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 8776d49.

@alexcjohnson
Copy link
Contributor

Alright, getting close 🙂 Aside from my latest comment (yes, always include all points in the event data) I only see two things:

(1) Currently when you’re in a selection mode and you click somewhere on the plot, whether you have a selection or not you get a selected event with no data. Can we just drop that event? ie if between mousedown and mouseup you didn't move the mouse enough to get a selecting event, don't emit a selected event.

(2) When you double click to deselect a region we don’t emit a selected event; we should, with just the remaining points. In fact when you double click to deselect everything, the first click gives an event (point 1 above, it shouldn't), but the second click which actually clears the selection doesn't give an event. It should, with no event data.

@archmoj
Copy link
Contributor Author

archmoj commented Jul 12, 2022

(2) When you double click to deselect a region we don’t emit a selected event; we should, with just the remaining points. In fact when you double click to deselect everything, the first click gives an event (point 1 above, it shouldn't), but the second click which actually clears the selection doesn't give an event. It should, with no event data.

Addressed in 8b959ea.
FYI - I also updated the state to avoid slightly confusing state you mentioned before after clicking the last selection of a subplot.

@archmoj
Copy link
Contributor Author

archmoj commented Jul 12, 2022

(1) Currently when you’re in a selection mode and you click somewhere on the plot, whether you have a selection or not you get a selected event with no data. Can we just drop that event? ie if between mousedown and mouseup you didn't move the mouse enough to get a selecting event, don't emit a selected event.

It looks to be related to this part:

// TODO: remove in v3 - this was probably never intended to work as it does,
// but in case anyone depends on it we don't want to break it now.
// Note that click-to-select introduced pre v3 also emitts proper
// event data when clickmode is having 'select' in its flag list.
gd.emit('plotly_selected', undefined);

@alexcjohnson While I am out please feel free if you wanted to push commits here :)

@alexcjohnson
Copy link
Contributor

I see - I had forgotten about that. I can't really see how someone would depend on that undefined event, but having made that choice before let's not change it now. I had also forgotten that we already have a plotly_deselect event. So with that in mind, it looks to me as though the PR right now has the events exactly right 🎉

After talking to @chriddyp just now, I think we should revisit the decision to have multiple selections be the default result when you drag multiple times, and instead go back to how it was before this PR: dragging a second time clears the first selection before creating a new one; if you want to add a second selection you need to shift-drag.

His main argument is that since this is how it worked before, most people probably never found the shift-drag functionality, so there's probably a lot of code out there that only supports a single selection region, and will appear broken if used with multiple regions. In addition, double-click to clear a selection might not be obvious to everyone, but getting stuck with one selection until you figure it out is a lot nicer than winding up with a dozen.

I think in the interest of time, in this PR we should simply change back to "you need to shift-drag to get a second selection." Down the line though we could add configurability, to either (a) keep that behavior, (b) disallow multiple selections entirely, or (c) the behavior you have right now, with no shift needed.

Another idea Chris had to make this easier is adding a delete button to each selection, that appears in the corner when you're hovering on the selection. But again, not needed for this PR, we can do this later.

@archmoj
Copy link
Contributor Author

archmoj commented Jul 13, 2022

After talking to @chriddyp just now, I think we should revisit the decision to have multiple selections be the default result when you drag multiple times, and instead go back to how it was before this PR: dragging a second time clears the first selection before creating a new one; if you want to add a second selection you need to shift-drag.

Addressed in feb4657.

@@ -1,27 +1,40 @@
'use strict';

var polybool = require('polybooljs');
var pointInPolygon = require('point-in-polygon/nested');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI we already have an "is this point inside this polygon" in polygon.tester

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. I tried to reuse it here: f28576e.
But it seems it require extra work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah it's old and undocumented... maybe I'll give it a shot myself at some point. Or maybe we drop that code entirely and just use point-in-polygon 😏

Copy link
Contributor

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work! Let's do it 😄 💃

@archmoj archmoj merged commit 41b0fd2 into master Jul 13, 2022
@archmoj archmoj deleted the selections branch July 13, 2022 21:41
@etpinard
Copy link
Contributor

Great feature - cheers 🍻

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.

None yet

3 participants