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

Fix shift selection after pan #2676

Merged
merged 6 commits into from
May 29, 2018
Merged

Conversation

etpinard
Copy link
Contributor

Fixes #2669

before:

peek 2018-05-28 17-51

now:

peek 2018-05-28 17-52

cc @alexcjohnson

@etpinard etpinard added bug something broken status: reviewable labels May 28, 2018
@@ -178,6 +178,8 @@ function makeDragBox(gd, plotinfo, x, y, w, h, ns, ew) {
prepSelect(e, startX, startY, dragOptions, dragModeNow);
} else {
dragOptions.clickFn = clickFn;
// clear selection polygon cache (if any)
plotinfo.selection = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be part of clearSelect? That gets called in all 3 blocks below (directly in the first and third, and in the second it's inside zoomPrep - looks like these can be simplified anyway) but also in zoomWheel - does that case have the same bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah. Good catch. Zoom has the problem.

Working on a fix ⛏️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed and 🔒 in f682fb8

if(redrawTimer === null) {
clearSelect(zoomlayer);
}
clearSelect(zoomlayer);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this fixes what I botched in 51c3128

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh wait, this worked fine in 51c3128

It is the optimization of bb02281 that broke this (except during the very first scroll event),

@@ -196,11 +194,18 @@ function makeDragBox(gd, plotinfo, x, y, w, h, ns, ew) {
} else if(dragModeNow === 'pan') {
dragOptions.moveFn = plotDrag;
dragOptions.doneFn = dragTail;
clearSelect(zoomlayer);
clearAndResetSelect();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, every branch here contains clearAndResetSelect (and this is the only caller of zoomPrep) so it can be factored out into just one call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooops, I missed the one in zoomPrep. Cleaned up in -> 34a0cf7

@alexcjohnson
Copy link
Collaborator

Fantastic. Really nice tests BTW! 💃

@etpinard etpinard merged commit c655f42 into master May 29, 2018
@etpinard etpinard deleted the fix-shift-selection-after-pan branch May 29, 2018 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants