Skip to content
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.

Sign up for GitHub

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

Fixes fill animation path #56

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fixes fill animation path #56

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
892c5c8
Fixes fill animation path
Mar 21, 2018
dc75635
fixed line drawing from coordinate 0 when fill isn't enabled
Apr 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions spark/src/main/java/com/robinhood/spark/SparkView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,10 +351,15 @@ public Path getSparkLinePath() {
/**
* Set the path to animate in onDraw, used for getAnimation purposes
*/
public void setAnimationPath(@NonNull Path animationPath) {
public void setAnimationPath(@NonNull final Path animationPath) {
float fillY = getFillEdge() != null ? getFillEdge() : 0;

this.renderPath.reset();
this.renderPath.addPath(animationPath);
this.renderPath.rLineTo(0, 0);
this.renderPath.rLineTo(0, fillType == FillType.UP ? -getHeight() : fillY);
Copy link
Contributor

Choose a reason for hiding this comment

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

super nit, but i think this can be simplified a bit to avoid needing to check for FillType.Up:

    // takes us straight up or down to the fill edge
    this.renderPath.rLineTo(0, fillEdge);
    // takes us straight over to the left edge
    this.renderPath.lineTo(0, fillEdge);
    // closes back to the path's start
    this.renderPath.close();

you end up having to check the fill-edge for null, and only do those operations if non-null. but i think it reads a bit cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking back over this, it actually seemed to be an issue with the getFillEdge() method, specifically:

case FillType.UP:
    return (float) getPaddingTop();

which wasn't taking into account the view height. Moving -getHeight() into this method and removing the ternary seemed to result in the same desired behavior. Though I'm not familiar with what else is using getFillEdge but local testing showed promising results.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I don't think we want the fill edge to be -getHeight() for the FillType.UP case. Conceptually, the fill edge should be the y value in drawing coordinates that we want to travel to to close the path. In the case of UP, it should be 0 since the top edge of a view in Android is coordinate 0. Then we have to take the padding into account, so we add paddingTop to that.

With that in mind, I think my initial code suggestion was wrong. We actually want something like:

            // line up or down to the fill edge
            sparkPath.lineTo(lastX, fillEdge);
            // line straight left to far edge of the view
            sparkPath.lineTo(getPaddingStart(), fillEdge);
            // closes line back on the first point
            sparkPath.close();

Which is from SparkView.populatePath(). Unfortunately, we don't have lastX handy. Also the lastX may be different from one animation to another. So, I think we may want to move the path closing/filling to the animators themselves.

if (fillType != FillType.NONE) {
this.renderPath.lineTo(0, fillY);
}

invalidate();
}
Expand Down