Skip to content

Return empty set for invisible bar trace #2081

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 3 commits into from
Oct 12, 2017
Merged

Conversation

monfera
Copy link
Contributor

@monfera monfera commented Oct 12, 2017

Downstream processing expects an array here, even when a trace is not visible:

                    var thisSelection = fillSelectionItem(
                        searchInfo.selectPoints(searchInfo, poly), searchInfo
                    );
                    if(selection.length) {
                        for(var j = 0; j < thisSelection.length; j++) { // <- won't have a length as selectPoints is undefined
                            selection.push(thisSelection[j]);
                        }
                    }

(side note: fillSelectionItem handles the case of a non-array selection, in which case it just passes it on ie. doesn't try to set curveNumber etc. properties on undefined; the result of which is the same undefined as the value of the first arg)

There's a similar return; in scatter, scattergeo, scattermapbox.

Is it a good direction @etpinard ?

@etpinard
Copy link
Contributor

etpinard commented Oct 12, 2017

Good catch.

I'd vote for making:

if(selection.length) { /* */

// turn into
if(selection && selection.length) { /* */

that way we won't have to remember to always return an array in all the trace module selectPoints methods.

@monfera
Copy link
Contributor Author

monfera commented Oct 12, 2017

@etpinard it's not selection but thisSelection that is undefined.

Also, we can't just do if(thisSelection && selection.length because there's an else branch too:

            function() {
                selection = [];
                for(i = 0; i < searchTraces.length; i++) {
                    searchInfo = searchTraces[i];
                    var thisSelection = fillSelectionItem(
                        searchInfo.selectPoints(searchInfo, poly), searchInfo
                    );
                    if(selection.length) {
                        for(var j = 0; j < thisSelection.length; j++) {
                            selection.push(thisSelection[j]);
                        }
                    }
                    else selection = thisSelection;
                }

                eventData = {points: selection};
                fillRangeItems(eventData, poly, pts);
                dragOptions.gd.emit('plotly_selecting', eventData);
            }

We probably don't want eventData.points to end up being undefined.

Looking at this block a bit more, it feels like it could be simplified: selection is initialized (to []) but its initial value is discarded in the else branch; also, @alexcjohnson is the purpose of the else branch to ensure selection be identical with thisSelection, and is it important? It probably has no impact on performance and the if/else could be simplified to just one straight code path if a shallow copy is fine too.

As resolution of this plotly.js issue but ancillary to crossfilter work I tried to avoid delving into it too deep, and always returning the same [...] type - set, or empty set, doesn't matter -
seemed elegant and of the smallest code impact.

@etpinard
Copy link
Contributor

etpinard commented Oct 12, 2017

always returning the same [...] type - set, or empty set, doesn't matter -
seemed elegant and of the smallest code impact.

Ok, if you think that's more elegant, I won't argue with you. Just make sure to lock that down in tests for all selectPoints methods.

@alexcjohnson alexcjohnson added the bug something broken label Oct 12, 2017
@alexcjohnson
Copy link
Collaborator

Good catch @monfera - I don't care exactly what route you take, but as long as the event always has a points array (even if it's empty) and it's robust against missing inputs, I'm happy.


// to make sure none of the above tests fail with extraneous invisible traces,
// add a bunch of them here
function addInvisible(fig, canHaveLegend) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Brilliant ✨

@etpinard
Copy link
Contributor

💃 nice tag-team effort on this one

💪 @monfera @alexcjohnson 💪

@alexcjohnson alexcjohnson merged commit f1faa28 into master Oct 12, 2017
@alexcjohnson alexcjohnson deleted the invisible-select branch October 12, 2017 21:29
@monfera
Copy link
Contributor Author

monfera commented Oct 12, 2017

@alexcjohnson incredible commit, thanks!

etpinard added a commit that referenced this pull request Oct 20, 2017
- PR #2099 got merged
  on a branch behind #2081
  which caused the test to fail on master.
etpinard added a commit that referenced this pull request Oct 23, 2017
- PR #2099 got merged
  on a branch behind #2081
  which caused the test to fail on master.
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.

3 participants