Selection plugin options #963

Merged
merged 3 commits into from Mar 1, 2013

Conversation

Projects
None yet
2 participants
Contributor

rlinehan commented Feb 19, 2013

Add two options for the selection plugin:

  1. Shape of the corners for the selection rectangle (defaults to "round).
  2. Always show the selection rectangle, even when the selected area is very small (defaults to "false"). If this option is set to true, the selection rectangle will show up as a line for very small selected areas.
Owner

dnschnur commented Feb 21, 2013

I like this, and it's actually small and clean enough to make it in 0.8.

I think I have an idea for the alwaysShow option, though: what if you instead introduced an option called minSize, replacing the constant in selectionIsSane? That would make it slightly more customizable, and apply it to all the sanity checks instead of just when drawing. What do you think?

Contributor

rlinehan commented Feb 22, 2013

I'm open to this idea, and like that it would be more customizable, but I have two slight issues with it: 1) it seems rather opaque what minSize actually means - or rather, what the calculations in selectionIsSane are actually getting. As a more concrete example, I'm working with a bar graph in time mode, with the BarWidth set to 1 minute. Being able to customize minSize would be useful to me if I knew what I would need to set minSize to so that I can select one bar without the selection rectangle going away. If we can write a decent explanation of what the number being put into minSize actually refers to, then I think this issue will be solved.

  1. Setting minSize to 0 means that 'plotunselected' events will not be triggered by clicking a graph. Thus, it is necessary to set minSize to a value, so there will always be a limit to how far you can zoom before the selection rectangle goes away (rather than alwaysShow: true, which doesn't have this limit). I guess I'm okay with this, again so long as minSize is better documented.
Owner

dnschnur commented Feb 26, 2013

That's a very good point regarding plotunselected events. I can see arguments either way, but in the end I think it makes a little more sense for visibility to be tied to selection. As long as the selection exists, even if it's very small, an unselected event feels unexpected.

Currently minSize is in pixels rather than axis units, so it doesn't work as-is for your 1-minute example. If we did switch it to axis units, I can see some users still wanting pixels. That would need further work anyway; in the case of bars, for example, you'd want not just a size corresponding to the bar width, but also the ability to align the selection with the bars in order to select in proper whole-bar increments.

So that makes more sense as a separate enhancement. Pixels are a good default, allow us to remain backwards-compatible, and prevent the immediate changes from becoming too broad. I definitely agree that it should be well-documented, with special mention of the side-effects of using a zero value.

rlinehan added some commits Feb 19, 2013

Add option for lineJoin shape
This commit adds an option for the shape of the corners of the
selection rectangle. By default the shape is set to "round" (the
previous setting for lineJoin). The other options are "bevel" and
"miter".
Add option to always show selection rectangle
Previously, if the selected area was very small, the selection
rectangle would not be displayed. This commit adds an "alwaysShow"
option so that, when true, the selection rectangle will always be
displayed. When the selected area is very small, the selection
rectangle will become a line.
Make minSize customizable
Previously, the minimum size a selection could be was set at five
pixels. This commit adds the ability to customize this value.
Contributor

rlinehan commented Feb 27, 2013

I've changed the behavior so that, as discussed, minSize is customizable. However, I do still think this fails to address the initial problem I encountered and why I implemented "alwaysShow". Say, as in the "visitors" and "zooming" examples, I have a placeholder graph and an overview graph. I can zoom in infinitely on the placeholder graph, so long as the size of my selection on the placeholder graph is a greater number of pixels than minSize. However, once my selection on the overview graph is smaller than minSize, the selection rectangle will disappear and I will no longer know where I am.

I guess really the behavior I want is for minSize to apply to both graphs simultaneously. This would require it to be in axis units/apply to the range, as you stated. Then I would be prevented from zooming in too far on the placeholder graph and on the overview graph. But as you said, this is a much broader change and not backwards compatible. For now I'll just have to make do with this implementation.

Owner

dnschnur commented Feb 27, 2013

Doesn't minSize help with that, though? If you set it to zero for the overview then you'll always have a thin selection there no matter how far in you zoom on the placeholder. If you want to proactively prevent someone from zooming in past a certain point, you could use the 'zoomrange' option of the navigate plugin, which is (kind of) expressed in axis units.

You do still lose the ability to click to clear the selection, although you could probably do that by listening for 'plotclick' events and calling plot.clearSelection yourself. That's admittedly starting to enter hack-y territory.

In any case, I think minSize makes a good option just in general, and the implementation is nice and clean, so I'll merge this pull request in the next couple of days. I would definitely encourage you to continue working on this in a new pull request or issue if it doesn't yet work the way you want it to.

Contributor

rlinehan commented Mar 1, 2013

minSize definitely helps, and for now I'm fine implementing slightly hacky things to get the behavior I want. Thanks for the suggestion to use zoomrange. Hopefully I'll get some time sometime soon to work on a solution that's not so hacky that I can submit another pull request for. Thanks!

@ghost ghost assigned dnschnur Mar 1, 2013

Owner

dnschnur commented Mar 1, 2013

Merging; thanks very much!

dnschnur added a commit that referenced this pull request Mar 1, 2013

@dnschnur dnschnur merged commit fe27116 into flot:master Mar 1, 2013

1 check failed

default The Travis build failed
Details

dnschnur added a commit that referenced this pull request Mar 2, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment