Skip to content

Introduce a new StepMode - #32

Merged
halfhp merged 8 commits into
halfhp:masterfrom
phisi:master
Feb 25, 2017
Merged

Introduce a new StepMode#32
halfhp merged 8 commits into
halfhp:masterfrom
phisi:master

Conversation

@phisi

@phisi phisi commented Feb 22, 2017

Copy link
Copy Markdown
Contributor

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

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
@halfhp

halfhp commented Feb 22, 2017

Copy link
Copy Markdown
Owner

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?

psi and others added 3 commits February 23, 2017 12:16
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
@phisi

phisi commented Feb 23, 2017

Copy link
Copy Markdown
Contributor Author

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

I am also thinking of making the PanZoom aware 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

halfhp commented Feb 23, 2017

Copy link
Copy Markdown
Owner

Sounds good!

@halfhp halfhp left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I made a couple minor comments that need to be addressed before merging but other than that, looks good.

Optional:
Would love to get a unit test or two for the new class, as well as an addition to the docs.

Comment thread .idea/copyright/profiles_settings.xml Outdated
<element module="All" copyright="AndroidPlot Apache 2.0" />
</module2copyright>
</settings>
<settings default="" />

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This file needs to be reverted before I can merge in.

Comment thread build.gradle Outdated

dependencies {
classpath 'com.android.tools.build:gradle:2.2.0'
classpath 'com.android.tools.build:gradle:2.2.3'

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

need to revert this change

Comment thread demoapp-wearable/build.gradle Outdated
}
dependencies {
classpath 'com.android.tools.build:gradle:1.2.3'
classpath 'com.android.tools.build:gradle:2.2.3'

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

need to revert this change too

* Depending on the currently displayed range (by value) choose increment so that
* the number of lines is closest to StepModel.value
*/

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

extra space should be removed. (not sure the javadoc generator will pick up the comment otherwise)

@phisi

phisi commented Feb 25, 2017

Copy link
Copy Markdown
Contributor Author

Ok that should do it...

@halfhp halfhp self-assigned this Feb 25, 2017
@halfhp
halfhp merged commit bae8096 into halfhp:master Feb 25, 2017
phisi added a commit to phisi/androidplot that referenced this pull request Feb 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants