PawletOS/app_Settings
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fixing bug in UsageGraph rendering.

Browse Source 
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
This commit is contained in:
Alex Kulesza
2017-07-30 18:11:24 -04:00
parent b79d38f2f3
commit 4a121ecfae
2 changed files with 180 additions and 21 deletions
Download Patch File Download Diff File Expand all files Collapse all files

47
src/com/android/settings/graph/UsageGraph.java
View File

@@ -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);

154
tests/robotests/src/com/android/settings/graph/UsageGraphTest.java Normal file
View File

@@ -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);
}
}