From 4a121ecfaed6393de254673a2d9d4bd74e475042 Mon Sep 17 00:00:00 2001 From: Alex Kulesza Date: Sun, 30 Jul 2017 18:11:24 -0400 Subject: [PATCH] Fixing bug in UsageGraph rendering. The calculateLocalPaths() method of UsageGraph converts a set of paths in (milliseconds, percent) coordinates into the actual pixel values that will be used for drawing. For the most part this is a one to one process, but not always: input points that are too closely spaced to draw accurately are skipped. The last point in the path, however, is never skipped, in order to ensure that the graph ends at the correct location. The previous implementation of this method had a bug: the y-coordinates of points that were skipped would be stored indefinitely (in the local variable pendingYLoc) and then added back at the very end of the path (under the condition i == mPaths.size() - 1 && pendingYLoc != PATH_DELIM). Under the right conditions, this led to the strange uptick at the end of the graph seen in the associated bug. This CL fixes the problem and attempts to make the logic slightly clearer. It also adds tests, one of which (_similarPointMiddle) fails for the previous code. In more detail, previously pendingXLoc was used to hold the last x coordinate seen, while pendingYLoc was used to hold the last *skipped* y coordinate, or PATH_DELIM otherwise. The difference between these was somewhat subtle and hard to understand from a quick read of the code, and there was a bug: pendingYLoc never got reset to PATH_DELIM even if later points were added. In this CL I have removed the pendingLoc variables in favor of a single lx/ly pair, which always holds the local coordinates of the most recent point, and I added an explicit boolean skippedLastPoint to track whether the point (lx, ly) has already been added or was skipped. Bug: 64065296 Test: make RunSettingsRoboTests Change-Id: I45ccffea1280d851bfae5143c2e84d188e133731 --- .../android/settings/graph/UsageGraph.java | 47 +++--- .../settings/graph/UsageGraphTest.java | 154 ++++++++++++++++++ 2 files changed, 180 insertions(+), 21 deletions(-) create mode 100644 tests/robotests/src/com/android/settings/graph/UsageGraphTest.java diff --git a/src/com/android/settings/graph/UsageGraph.java b/src/com/android/settings/graph/UsageGraph.java index 5a4a9cd6393..a39cb43e27b 100644 --- a/src/com/android/settings/graph/UsageGraph.java +++ b/src/com/android/settings/graph/UsageGraph.java @@ -28,8 +28,8 @@ import android.graphics.Paint.Style; import android.graphics.Path; import android.graphics.Shader.TileMode; import android.graphics.drawable.Drawable; +import android.support.annotation.VisibleForTesting; import android.util.AttributeSet; -import android.util.Log; import android.util.SparseIntArray; import android.util.TypedValue; import android.view.View; @@ -93,7 +93,7 @@ public class UsageGraph extends View { float dots = resources.getDimensionPixelSize(R.dimen.usage_graph_dot_size); float interval = resources.getDimensionPixelSize(R.dimen.usage_graph_dot_interval); mDottedPaint.setStrokeWidth(dots * 3); - mDottedPaint.setPathEffect(new DashPathEffect(new float[]{dots, interval}, 0)); + mDottedPaint.setPathEffect(new DashPathEffect(new float[] {dots, interval}, 0)); mDottedPaint.setColor(context.getColor(R.color.usage_graph_dots)); TypedValue v = new TypedValue(); @@ -136,8 +136,8 @@ public class UsageGraph extends View { addPathAndUpdate(points, mProjectedPaths, mLocalProjectedPaths); } - private void addPathAndUpdate(SparseIntArray points, SparseIntArray paths, - SparseIntArray localPaths) { + private void addPathAndUpdate( + SparseIntArray points, SparseIntArray paths, SparseIntArray localPaths) { final long startTime = System.currentTimeMillis(); for (int i = 0, size = points.size(); i < size; i++) { paths.put(points.keyAt(i), points.valueAt(i)); @@ -170,41 +170,44 @@ public class UsageGraph extends View { calculateLocalPaths(mProjectedPaths, mLocalProjectedPaths); } - private void calculateLocalPaths(SparseIntArray paths, SparseIntArray localPaths) { + @VisibleForTesting + void calculateLocalPaths(SparseIntArray paths, SparseIntArray localPaths) { final long startTime = System.currentTimeMillis(); if (getWidth() == 0) { return; } localPaths.clear(); - int pendingXLoc = 0; - int pendingYLoc = PATH_DELIM; + // Store the local coordinates of the most recent point. + int lx = 0; + int ly = PATH_DELIM; + boolean skippedLastPoint = false; for (int i = 0; i < paths.size(); i++) { int x = paths.keyAt(i); int y = paths.valueAt(i); if (y == PATH_DELIM) { - if (i == paths.size() - 1 && pendingYLoc != PATH_DELIM) { - // Connect to the end of the graph. - localPaths.put(pendingXLoc, pendingYLoc); + if (i == paths.size() - 1 && skippedLastPoint) { + // Add back skipped point to complete the path. + localPaths.put(lx, ly); } - // Clear out any pending points. - pendingYLoc = PATH_DELIM; - localPaths.put(pendingXLoc + 1, PATH_DELIM); + skippedLastPoint = false; + localPaths.put(lx + 1, PATH_DELIM); } else { - final int lx = getX(x); - final int ly = getY(y); - pendingXLoc = lx; + lx = getX(x); + ly = getY(y); + // Skip this point if it is not far enough from the last one added. if (localPaths.size() > 0) { int lastX = localPaths.keyAt(localPaths.size() - 1); int lastY = localPaths.valueAt(localPaths.size() - 1); if (lastY != PATH_DELIM && !hasDiff(lastX, lx) && !hasDiff(lastY, ly)) { - pendingYLoc = ly; + skippedLastPoint = true; continue; } } + skippedLastPoint = false; localPaths.put(lx, ly); } } - BatteryUtils.logRuntime(LOG_TAG,"calculateLocalPaths", startTime); + BatteryUtils.logRuntime(LOG_TAG, "calculateLocalPaths", startTime); } private boolean hasDiff(int x1, int x2) { @@ -221,8 +224,8 @@ public class UsageGraph extends View { private void updateGradient() { mFillPaint.setShader( - new LinearGradient(0, 0, 0, getHeight(), getColor(mAccentColor, .2f), 0, - TileMode.CLAMP)); + new LinearGradient( + 0, 0, 0, getHeight(), getColor(mAccentColor, .2f), 0, TileMode.CLAMP)); } private int getColor(int color, float alphaScale) { @@ -236,7 +239,9 @@ public class UsageGraph extends View { if (mMiddleDividerLoc != 0) { drawDivider(0, canvas, mTopDividerTint); } - drawDivider((int) ((canvas.getHeight() - mDividerSize) * mMiddleDividerLoc), canvas, + drawDivider( + (int) ((canvas.getHeight() - mDividerSize) * mMiddleDividerLoc), + canvas, mMiddleDividerTint); drawDivider(canvas.getHeight() - mDividerSize, canvas, -1); diff --git a/tests/robotests/src/com/android/settings/graph/UsageGraphTest.java b/tests/robotests/src/com/android/settings/graph/UsageGraphTest.java new file mode 100644 index 00000000000..fbd6fd487e5 --- /dev/null +++ b/tests/robotests/src/com/android/settings/graph/UsageGraphTest.java @@ -0,0 +1,154 @@ +/* + * Copyright (C) 2017 The Android Open Source Project + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * + */ + +package com.android.settings.graph; + +import static com.google.common.truth.Truth.assertThat; + +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.spy; + +import android.content.Context; +import android.content.res.Resources; +import android.util.SparseIntArray; + +import com.android.settings.TestConfig; +import com.android.settingslib.R; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.robolectric.RobolectricTestRunner; +import org.robolectric.RuntimeEnvironment; +import org.robolectric.annotation.Config; + +@RunWith(RobolectricTestRunner.class) +@Config(manifest = TestConfig.MANIFEST_PATH, sdk = TestConfig.SDK_VERSION) +public class UsageGraphTest { + private UsageGraph mGraph; + + @Before + public void setUp() { + // Set up a graph view of width 1000, height 200, and corner radius 5. + Context context = spy(RuntimeEnvironment.application); + Resources resources = spy(context.getResources()); + doReturn(resources).when(context).getResources(); + doReturn(5).when(resources).getDimensionPixelSize(R.dimen.usage_graph_line_corner_radius); + doReturn(1).when(resources).getDimensionPixelSize(R.dimen.usage_graph_line_width); + doReturn(1).when(resources).getDimensionPixelSize(R.dimen.usage_graph_dot_size); + doReturn(1).when(resources).getDimensionPixelSize(R.dimen.usage_graph_dot_interval); + doReturn(1).when(resources).getDimensionPixelSize(R.dimen.usage_graph_divider_size); + mGraph = spy(new UsageGraph(context, null)); + doReturn(1000).when(mGraph).getWidth(); + doReturn(200).when(mGraph).getHeight(); + + // Set the conceptual size of the graph to 500ms x 100%. + mGraph.setMax(500, 100); + } + + @Test + public void testCalculateLocalPaths_singlePath() { + SparseIntArray paths = new SparseIntArray(); + paths.append(0, 100); + paths.append(500, 50); + paths.append(501, -1); + + SparseIntArray localPaths = new SparseIntArray(); + mGraph.calculateLocalPaths(paths, localPaths); + + assertThat(localPaths.size()).isEqualTo(3); + assertThat(localPaths.keyAt(0)).isEqualTo(0); + assertThat(localPaths.valueAt(0)).isEqualTo(0); + assertThat(localPaths.keyAt(1)).isEqualTo(1000); + assertThat(localPaths.valueAt(1)).isEqualTo(100); + assertThat(localPaths.keyAt(2)).isEqualTo(1001); + assertThat(localPaths.valueAt(2)).isEqualTo(-1); + } + + @Test + public void testCalculateLocalPaths_multiplePaths() { + SparseIntArray paths = new SparseIntArray(); + paths.append(0, 100); + paths.append(200, 75); + paths.append(201, -1); + + paths.append(300, 50); + paths.append(500, 25); + paths.append(501, -1); + + SparseIntArray localPaths = new SparseIntArray(); + mGraph.calculateLocalPaths(paths, localPaths); + + assertThat(localPaths.size()).isEqualTo(6); + + assertThat(localPaths.keyAt(0)).isEqualTo(0); + assertThat(localPaths.valueAt(0)).isEqualTo(0); + assertThat(localPaths.keyAt(1)).isEqualTo(400); + assertThat(localPaths.valueAt(1)).isEqualTo(50); + assertThat(localPaths.keyAt(2)).isEqualTo(401); + assertThat(localPaths.valueAt(2)).isEqualTo(-1); + + assertThat(localPaths.keyAt(3)).isEqualTo(600); + assertThat(localPaths.valueAt(3)).isEqualTo(100); + assertThat(localPaths.keyAt(4)).isEqualTo(1000); + assertThat(localPaths.valueAt(4)).isEqualTo(150); + assertThat(localPaths.keyAt(5)).isEqualTo(1001); + assertThat(localPaths.valueAt(5)).isEqualTo(-1); + } + + @Test + public void testCalculateLocalPaths_similarPointMiddle() { + SparseIntArray paths = new SparseIntArray(); + paths.append(0, 100); + paths.append(1, 99); // This point should be omitted. + paths.append(500, 50); + paths.append(501, -1); + + SparseIntArray localPaths = new SparseIntArray(); + mGraph.calculateLocalPaths(paths, localPaths); + + assertThat(localPaths.size()).isEqualTo(3); + assertThat(localPaths.keyAt(0)).isEqualTo(0); + assertThat(localPaths.valueAt(0)).isEqualTo(0); + assertThat(localPaths.keyAt(1)).isEqualTo(1000); + assertThat(localPaths.valueAt(1)).isEqualTo(100); + assertThat(localPaths.keyAt(2)).isEqualTo(1001); + assertThat(localPaths.valueAt(2)).isEqualTo(-1); + } + + @Test + public void testCalculateLocalPaths_similarPointEnd() { + SparseIntArray paths = new SparseIntArray(); + paths.append(0, 100); + paths.append(499, 51); + paths.append(500, 50); // This point should be kept: it's the last one. + paths.append(501, -1); + + SparseIntArray localPaths = new SparseIntArray(); + mGraph.calculateLocalPaths(paths, localPaths); + + assertThat(localPaths.size()).isEqualTo(4); + assertThat(localPaths.keyAt(0)).isEqualTo(0); + assertThat(localPaths.valueAt(0)).isEqualTo(0); + assertThat(localPaths.keyAt(1)).isEqualTo(998); + assertThat(localPaths.valueAt(1)).isEqualTo(98); + assertThat(localPaths.keyAt(2)).isEqualTo(1000); + assertThat(localPaths.valueAt(2)).isEqualTo(100); + assertThat(localPaths.keyAt(3)).isEqualTo(1001); + assertThat(localPaths.valueAt(3)).isEqualTo(-1); + } +}