Skip to content

1852 persistent click via select points #2944

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

Merged
merged 56 commits into from
Sep 10, 2018
Merged
Show file tree
Hide file tree
Changes from 47 commits
Commits
Show all changes
56 commits
Select commit Hold shift + click to select a range
2fbaaaa
Tracer-bullet impl of fast click-to-select [1852]
rmoestl Aug 10, 2018
346bad9
Set correct blank outlines path in case of zero select polygons [1852]
rmoestl Aug 16, 2018
4f8dd92
Fix select_test.js Jasmine tests [1852]
rmoestl Aug 16, 2018
6a87769
Add click-to-select specific tests to select_test.js [1852]
rmoestl Aug 17, 2018
2d59365
Make click-to-select work in pan/zoom dragmodes [1852]
rmoestl Aug 21, 2018
5a13573
Introduce layout attribute 'clickmode' [1852]
rmoestl Aug 21, 2018
d3b4a0f
Simplify signature of selection testers' contains function [1852]
rmoestl Aug 21, 2018
36145df
Support click-to-select for histograms [1852]
rmoestl Aug 22, 2018
73e5085
Fix click-to-select bug when clearing selection with Shift [1852]
rmoestl Aug 22, 2018
1a89040
Enable click-to-select for scattergl in pan/zoom mode [1852]
rmoestl Aug 22, 2018
a6e49fd
Add click-to-select support to box plots [1852]
rmoestl Aug 22, 2018
bb44b6d
Fix bug when clearing selections and add/subtract mode on [1852]
rmoestl Aug 23, 2018
48a0a36
Test violin trace type's support for click-to-test [1852]
rmoestl Aug 23, 2018
4639718
Add click-to-select support to ohlc and candlestick [1852]
rmoestl Aug 23, 2018
497747b
Add click-to-select support to geo base plot [1852]
rmoestl Aug 23, 2018
744ab56
Add click-to-select support to mapbox and scattermapbox [1852]
rmoestl Aug 24, 2018
7da973b
Add @flaky flag to click-to-select tests [1852]
rmoestl Aug 24, 2018
ee5f990
Reactivate unhover on mouseout event in geo base plot [1852]
rmoestl Aug 24, 2018
6ac1a08
Add click-to-select support to scattergeo [1852]
rmoestl Aug 24, 2018
e11faf5
Add click-to-select support to ternary base plot [1852]
rmoestl Aug 24, 2018
bef47a3
Add test of scattercarpet's click-to-select support [1852]
rmoestl Aug 24, 2018
c394a8b
Add click-to-select support to polar base plot [1852]
rmoestl Aug 24, 2018
5b08a2c
Add testing scatterpolargl's click-to-select support [1852]
rmoestl Aug 24, 2018
e763964
Add click-to-select support to splom [1852]
rmoestl Aug 24, 2018
41e3ba5
Activate persistent selection for ternary plots [1852]
rmoestl Aug 27, 2018
6a2ae3b
Refine click-to-select TODO comments that'll be solved later [1852]
rmoestl Aug 27, 2018
e7c2240
Clean up box' custom eventData function [1852]
rmoestl Aug 27, 2018
23024b2
Call click-to-select's isPointOrBinSelected as late as possible [1852]
rmoestl Aug 27, 2018
cbd9f07
Refactor selection testing namings and structure [1852]
rmoestl Aug 27, 2018
7eba640
Add comment for multiTester's contains function [1852]
rmoestl Aug 27, 2018
92ee0fa
Remove two obsolete comments from cartesian/select.js [1852]
rmoestl Aug 27, 2018
3073f61
Merge branch 'master' into 1852-persistent-click-via-selectPoints
rmoestl Aug 27, 2018
3767ce2
Fix test-syntax linting errors in select_test.js [1852]
rmoestl Aug 27, 2018
8a4fc99
Fix further test-syntax linting errors in select_test.js [1852]
rmoestl Aug 27, 2018
db3e126
Fix for-loop bug in cartesian/select.js caught by ESLint [1852]
rmoestl Aug 27, 2018
b3984f7
Adapt description of of `clickmode` attribute [1852]
rmoestl Aug 29, 2018
2f7ad27
Simplify check if clicked point is the sole selected point [1852]
rmoestl Aug 30, 2018
d7c1434
Flag click-to-select tests of gl-based traces with @gl [1852]
rmoestl Aug 30, 2018
e895efd
Rename variable in `lib/select.js` [1852]
rmoestl Aug 30, 2018
60317b1
Adapt uninitialized var declarations in click-to-select code [1852]
rmoestl Aug 30, 2018
61b1a32
Add 'none' as an extra flag to clickmode [1852]
rmoestl Aug 30, 2018
e460e46
Simplify code for coercing selection cache state [1852]
rmoestl Aug 30, 2018
2ef3fce
Resolve TODO in cartesian/select.js regarding binned traces [1852]
rmoestl Aug 30, 2018
ac4b909
Remove lib/select.js and integrate code in cartesian/select.js [1852]
rmoestl Aug 30, 2018
05b50b9
Fix mapbox click-to-select bug caused by multiple click listeners [1852]
rmoestl Aug 30, 2018
88e9993
Fix testing click-to-select is inactive if clickmode is 'event' [1852]
rmoestl Sep 4, 2018
f247d93
Ensure 'plotly_click' is fired after 'plotly_selected' [1852]
rmoestl Sep 4, 2018
90ba209
Replace dynamic CI tags with static ones in select_test.js [1852]
rmoestl Sep 5, 2018
2bc582b
Apply @noCI tag to mapbox tests in select_test.js [1852]
rmoestl Sep 5, 2018
96fcdd7
Minor change: move helper method in select_test.s [1852]
rmoestl Sep 5, 2018
a5a90f9
Add a comment to select_test.js test [1852]
rmoestl Sep 6, 2018
87d0497
Default hovermode to closest if clickmode is select [1852]
rmoestl Sep 7, 2018
30789dd
Remove :hocho: obsolete polygon.multitester function [1852]
rmoestl Sep 7, 2018
cc2c9ba
Merge branch 'master' into 1852-persistent-click-via-selectPoints
rmoestl Sep 7, 2018
32aa219
Simplify clickmode / hovermode coercion code [1852]
rmoestl Sep 7, 2018
b6bd813
Not throw error for no longer valid polygon.tester argument [1852]
rmoestl Sep 10, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions src/components/fx/layout_attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,29 @@ fontAttrs.family.dflt = constants.HOVERFONT;
fontAttrs.size.dflt = constants.HOVERFONTSIZE;

module.exports = {
clickmode: {
valType: 'flaglist',
role: 'info',
flags: ['event', 'select'],
dflt: 'event',
editType: 'plot',
extras: ['none'],
description: [
'Determines the mode of single click interactions.',
'*event* is the default value and emits the `plotly_click`',
'event. In addition this mode emits the `plotly_selected` event',
'in drag modes *lasso* and *select*, but with no event data attached',
'(kept for compatibility reasons).',
'The *select* flag enables selecting single',
'data points via click. This mode also supports persistent selections,',
'meaning that pressing Shift while clicking, adds to / subtracts from an',
'existing selection. *select* with `hovermode`: *x* can be confusing, consider',
'explicitly setting `hovermode`: *closest* when using this feature.',
'Selection events are sent accordingly as long as *event* flag is set as well.',
'When the *event* flag is missing, `plotly_click` and `plotly_selected`',
'events are not fired.'
].join(' ')
},
dragmode: {
valType: 'enumerated',
role: 'info',
Expand Down
2 changes: 2 additions & 0 deletions src/components/fx/layout_defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) {
return Lib.coerce(layoutIn, layoutOut, layoutAttributes, attr, dflt);
}

coerce('clickmode');
Copy link
Contributor

@etpinard etpinard Sep 6, 2018

Choose a reason for hiding this comment

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

We're still missing @alexcjohnson's smart default suggestion where when a user sets clickmode: 'select' the default hovermode value becomes 'closest'.

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, sorry, I misunderstood that. I thought mentioning in clickmode's description that hovermode set to closest is best for most applications was "enough".

So what we want to have is when clickmode includes the select flag and nothing is set explicitly for hovermode, the default of hovermode should be closest. Is that right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

So what we want to have is when clickmode includes the select flag and nothing is set explicitly for hovermode, the default of hovermode should be closest. Is that right?

Yep! Which should be as simple as modifying the hovermodeDflt logic below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!


var dragMode = coerce('dragmode');
if(dragMode === 'select') coerce('selectdirection');

Expand Down
1 change: 1 addition & 0 deletions src/lib/polygon.js
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ polygon.tester = function tester(ptsIn) {
};
};

// TODO Somewhat redundant to multiTester in 'lib/select.js'
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think polygon.multiTester gets called anywhere now. Would you mind 🔪 ing it?

  • Extract multitester to new module lib/select.js because
    multitester is no longer to been seen in the
    sole context of polygons.

This makes perfect sense. Using a polygon multi-tester function for click-to-select would have been confusing 👌

That said, I don't think we need another file in lib/ for this thing. Plus, having files called select.js in plots/cartesian, all over traces/ and now lib/select.js probably isn't ideal. Would it be ok to just move the contents of your new lib/select.js to cartesian/select.js?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be ok to just move the contents of your new lib/select.js to cartesian/select.js?

It's not just used by cartesian though, right?

Copy link
Contributor Author

@rmoestl rmoestl Aug 30, 2018

Choose a reason for hiding this comment

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

It's not just used by cartesian though, right?

Exactly. E.g. it is used in polar and choropleth.
Update: Sorry, I was referring to polygon.tester. cartesian/select.js is in fact the only module requiring lib/select.js. So I think it'd be better to move it there. Do you agree, @alexcjohnson ?

I don't think polygon.multiTester gets called anywhere now

Nope. It's called in polygon.tester in the first line.
Update: polygon.tester is used by polar, choropleth etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, all of cartesian/select.js should be moved out of cartesian/ - as @alexcjohnson said it is not just used for cartesian subplots anymore.

What I was suggesting is to move the contents of lib/select.js into the same file as cartesian/select.js as the contents of lib/select.js aren't used anywhere else.

Once that's done, we can move cartesian/select.js to either components/fx (side-by-side with the hover routines) or in a new components/ directory e.g. components/select.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Moved contents of lib/select.js into cartesian/select.js`.

Copy link
Contributor

@etpinard etpinard Sep 6, 2018

Choose a reason for hiding this comment

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

@rmoestl thanks for moving the contents to cartesian/select.js 🚚

Now, would you mind 🔪 the multiTester routine and the TODO above from this file. polygon.multiTester isn't used anywhere now.

Copy link
Contributor Author

@rmoestl rmoestl Sep 6, 2018

Choose a reason for hiding this comment

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

Sorry, to repeat that, but isnt't polygon.multiTester called here

if(Array.isArray(ptsIn[0][0])) return polygon.multitester(ptsIn);
?

Or can we 🔪 that line as well? I hesitated to do so back then because polygon.tester is called from multiple places other than selection code - potentially with a parameter type that requires this very line.

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad! I grep'ed for multiTester not multitester. You're correct, multitester is currently used in tester in lib/polygon.js.

Now, if I recall correctly, that polygon.multitester call in polygon.tester was only intended for mulit-selections. Git blame points us to b800e55 which is consistent.

When commenting out this line

if(Array.isArray(ptsIn[0][0])) return polygon.multitester(ptsIn);

no test fail.

So yeah, I think it's safe to 🔪 everything about multitester in lib/polygon.js.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I do think we have enough tests that you could try replacing that line with
if(Array.isArray(ptsIn[0][0])) throw new Error('multitester called!');
or something and see if it ever happens.

But if it does, could that line just point to the new multiTester? Of course doing that would make a circular reference unless these are brought into the same file - which might be better anyway, I feel like these belong together (even if we can't quite agree on where that together should be...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Running all tests didn't throw the error. I left throwing the error in place and added a nice comment to it, so that future readers understand the "why".

/**
* Test multiple polygons
*/
Expand Down
29 changes: 27 additions & 2 deletions src/plots/cartesian/dragbox.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ var doTicksSingle = require('./axes').doTicksSingle;
var getFromId = require('./axis_ids').getFromId;
var prepSelect = require('./select').prepSelect;
var clearSelect = require('./select').clearSelect;
var selectOnClick = require('./select').selectOnClick;
var scaleZoom = require('./scale_zoom');

var constants = require('./constants');
Expand Down Expand Up @@ -148,7 +149,11 @@ function makeDragBox(gd, plotinfo, x, y, w, h, ns, ew) {
};

dragOptions.prepFn = function(e, startX, startY) {
var dragModePrev = dragOptions.dragmode;
var dragModeNow = gd._fullLayout.dragmode;
if(dragModeNow !== dragModePrev) {
dragOptions.dragmode = dragModeNow;
}

recomputeAxisLists();

Expand Down Expand Up @@ -178,7 +183,19 @@ function makeDragBox(gd, plotinfo, x, y, w, h, ns, ew) {
prepSelect(e, startX, startY, dragOptions, dragModeNow);
} else {
dragOptions.clickFn = clickFn;
clearAndResetSelect();
if(isSelectOrLasso(dragModePrev)) {
// TODO Fix potential bug
// Note: clearing / resetting selection state only happens, when user
// triggers at least one interaction in pan/zoom mode. Otherwise, the
// select/lasso outlines are deleted (in plots.js.cleanPlot) but the selection
// cache isn't cleared. So when the user switches back to select/lasso and
// 'adds to a selection' with Shift, the "old", seemingly removed outlines
// are redrawn again because the selection cache still holds their coordinates.
// However, this isn't easily solved, since plots.js would need
// to have a reference to the dragOptions object (which holds the
// selection cache).
clearAndResetSelect();
}

if(!allFixedRanges) {
if(dragModeNow === 'zoom') {
Expand Down Expand Up @@ -207,12 +224,20 @@ function makeDragBox(gd, plotinfo, x, y, w, h, ns, ew) {
}

function clickFn(numClicks, evt) {
var clickmode = gd._fullLayout.clickmode;

removeZoombox(gd);

if(numClicks === 2 && !singleEnd) doubleClick();

if(isMainDrag) {
Fx.click(gd, evt, plotinfo.id);
if(clickmode.indexOf('select') > -1) {
selectOnClick(evt, gd, xaxes, yaxes, plotinfo.id, dragOptions);
}

if(clickmode.indexOf('event') > -1) {
Fx.click(gd, evt, plotinfo.id);
}
}
else if(numClicks === 1 && singleEnd) {
var ax = ns ? ya0 : xa0,
Expand Down
Loading