1852 persistent click via select points #2944
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,116 @@ | ||
| /** | ||
| * Copyright 2012-2018, Plotly, Inc. | ||
| * All rights reserved. | ||
| * | ||
| * This source code is licensed under the MIT license found in the | ||
| * LICENSE file in the root directory of this source tree. | ||
| */ | ||
|
|
||
|
|
||
| 'use strict'; | ||
|
|
||
| var polygon = require('./polygon'); | ||
|
|
||
| var select = module.exports = {}; | ||
|
|
||
| /** | ||
| * Constructs a new point selection definition. | ||
| * | ||
| * @param pointNumber the point number of the point as in `data` | ||
| * @param searchInfo identifies the trace the point is contained in | ||
| * @param subtract true if the selection definition should mark a deselection | ||
| * @return {{pointNumber: Number, searchInfo: Object, subtract: boolean}} | ||
| */ | ||
| select.pointSelectionDef = function(pointNumber, searchInfo, subtract) { | ||
| return { | ||
| pointNumber: pointNumber, | ||
| searchInfo: searchInfo, | ||
| subtract: subtract | ||
| }; | ||
| }; | ||
|
|
||
| function isPointSelectionDef(o) { | ||
| return 'pointNumber' in o && 'searchInfo' in o; | ||
| } | ||
|
|
||
| function pointNumberTester(pointSelectionDef) { | ||
| return { | ||
| xmin: 0, | ||
| xmax: 0, | ||
| ymin: 0, | ||
| ymax: 0, | ||
| pts: [], | ||
| contains: function(pt, omitFirstEdge, pointNumber, searchInfo) { | ||
| return searchInfo.cd[0].trace._expandedIndex === pointSelectionDef.searchInfo.cd[0].trace._expandedIndex && | ||
| pointNumber === pointSelectionDef.pointNumber; | ||
| }, | ||
| isRect: false, | ||
| degenerate: false, | ||
| subtract: pointSelectionDef.subtract | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Wraps multiple selection testers. | ||
| * | ||
| * @param list an array of selection testers | ||
| * | ||
| * @return a selection tester object with a contains function | ||
| * that can be called to evaluate a point against all wrapped | ||
| * selection testers that were passed in list. | ||
| */ | ||
| select.multiTester = function multiTester(list) { | ||
| var testers = []; | ||
| var xmin = isPointSelectionDef(list[0]) ? 0 : list[0][0][0]; | ||
| var xmax = xmin; | ||
| var ymin = isPointSelectionDef(list[0]) ? 0 : list[0][0][1]; | ||
| var ymax = ymin; | ||
|
|
||
| for(var i = 0; i < list.length; i++) { | ||
| if(isPointSelectionDef(list[i])) { | ||
| testers.push(pointNumberTester(list[i])); | ||
| } else { | ||
| var tester = polygon.tester(list[i]); | ||
| tester.subtract = list[i].subtract; | ||
| testers.push(tester); | ||
| xmin = Math.min(xmin, tester.xmin); | ||
| xmax = Math.max(xmax, tester.xmax); | ||
| ymin = Math.min(ymin, tester.ymin); | ||
| ymax = Math.max(ymax, tester.ymax); | ||
| } | ||
| } | ||
|
|
||
| // TODO Consider making signature of contains more lean | ||
| /** | ||
| * Tests if the given point is within this tester. | ||
| * | ||
| * @param pt an object having an `x` and a `y` property defining the location | ||
| * of the point | ||
| * @param arg parameter to pass additional arguments down to wrapped testers | ||
| * @param pointNumber the point number of the point | ||
| * @param searchInfo identifies the trace the point is contained in | ||
| * @return {boolean} | ||
| */ | ||
| function contains(pt, arg, pointNumber, searchInfo) { | ||
| var yes = false; | ||
|
There was a problem hiding this comment. 🐄 I'd use a somewhat less prejudiced name - There was a problem hiding this comment. Sorry, I wasn't looking through that code too much since I just moved There was a problem hiding this comment. Ah, I hadn't noticed that it was already there before. Well, moving is the best time to clean house :) thanks! |
||
| for(var i = 0; i < testers.length; i++) { | ||
| if(testers[i].contains(pt, arg, pointNumber, searchInfo)) { | ||
| // if contained by subtract tester - exclude the point | ||
| yes = testers[i].subtract === false; | ||
| } | ||
| } | ||
|
|
||
| return yes; | ||
| } | ||
|
|
||
| return { | ||
| xmin: xmin, | ||
| xmax: xmax, | ||
| ymin: ymin, | ||
| ymax: ymax, | ||
| pts: [], | ||
| contains: contains, | ||
| isRect: false, | ||
| degenerate: false | ||
| }; | ||
| }; | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.multiTestergets called anywhere now. Would you mind 🔪 ing it?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 calledselect.jsinplots/cartesian, all overtraces/and nowlib/select.jsprobably isn't ideal. Would it be ok to just move the contents of your newlib/select.jstocartesian/select.js?There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. E.g. it is used inpolarandchoropleth.Update: Sorry, I was referring to
polygon.tester.cartesian/select.jsis in fact the only module requiringlib/select.js. So I think it'd be better to move it there. Do you agree, @alexcjohnson ?Nope. It's called in
polygon.testerin the first line.Update:
polygon.testeris used bypolar,choroplethetc.There was a problem hiding this comment.
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.jsshould be moved out ofcartesian/- as @alexcjohnson said it is not just used for cartesian subplots anymore.What I was suggesting is to move the contents of
lib/select.jsinto the same file ascartesian/select.jsas the contents oflib/select.jsaren't used anywhere else.Once that's done, we can move
cartesian/select.jsto eithercomponents/fx(side-by-side with the hover routines) or in a newcomponents/directory e.g.components/select.There was a problem hiding this comment.
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.jsinto cartesian/select.js`.There was a problem hiding this comment.
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
multiTesterroutine and theTODOabove from this file.polygon.multiTesterisn't used anywhere now.There was a problem hiding this comment.
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.multiTestercalled hereplotly.js/src/lib/polygon.js
Line 34 in cbd9f07
Or can we 🔪 that line as well? I hesitated to do so back then because
polygon.testeris called from multiple places other than selection code - potentially with a parameter type that requires this very line.There was a problem hiding this comment.
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
multiTesternotmultitester. You're correct,multitesteris currently used intesterinlib/polygon.js.Now, if I recall correctly, that
polygon.multitestercall inpolygon.testerwas only intended for mulit-selections. Git blame points us to b800e55 which is consistent.When commenting out this line
plotly.js/src/lib/polygon.js
Line 34 in cbd9f07
no test fail.
So yeah, I think it's safe to 🔪 everything about
multitesterinlib/polygon.js.There was a problem hiding this comment.
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...)There was a problem hiding this comment.
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".