halfhp / androidplot

Charts and plots for Android
http://androidplot.com
Apache License 2.0
514 stars 159 forks source link

Introduce a new StepMode #32

Closed phisi closed 7 years ago

phisi commented 7 years ago

Why: Increment by value -> no good when zooming Increment by pixel -> no good when zooming subdivide -> depending on data chooses ticks at wired locations e.g (1.3 , 2.3, 3,3 instead of 1, 2, 3)

Workaround: When you know your data: supply an array of predefined increments (by value) for StepModel to choose from to best fit the desired number of lines. For example: Start zoomed out with ticks every 100 and as you zoom in switch to 50,10,1

Also: Sorry about the changed gradle/profile settings disregard those

halfhp commented 7 years ago

I really love this feature - thanks for contributing!

One comment on the design: Could this be refactored to avoid adding logic in XYPlot that has to inspect the StepModel in order to determine the right way to extract a return value for getRangeStepValue() / getDomainStepValue()? One approach might be to create a subclass of XYStepModel that takes the XYPlot instance as a param and uses it to internally calculate the result of getValue().

I suggest this because XYPlot.getRangeStepValue() & XYPlot.getDomainStepValue() are not only two consumers of the XYPlot instance's StepModel. Repeating the if (domainStepModel.getMode() == StepMode.INCREMENT_BY_FIT) {...} logic in each case is not particularly DRY. Additionally, because XYPlot provides a public getter for the StepModel, third parties wishing to use this new mode would need to include this check in their code as well.

Thoughts?

phisi commented 7 years ago

Yeah, subclassing is a better idea (see merge). XYPlot is now unchanged. I do not pass the entire plot to the StepModelbut the respective plot.getBounds().getxRegion(), it does not need to know more.

I am also thinking of making the PanZoomaware of this mode (and possibly the normal INCREMENT_BY_VALUE) so the zoom stops when only a single tick is visible. Zooming further than that seems pointless. But that is for another pull request i think.

halfhp commented 7 years ago

Sounds good!

phisi commented 7 years ago

Ok that should do it...