From 3fc44b9a6274254e8d3a53b6b1e245c5f9177229 Mon Sep 17 00:00:00 2001 From: Felipe Leme Date: Mon, 21 Mar 2016 17:34:21 -0700 Subject: [PATCH] Changed logic when dumpstate's max progress increases. When dumpstate starts, it estimates its maximum duration and sends it through an extra on BUGREPORT_STARTED; as it progress, it sets a system property with its current progress and if the progress value overflows the estimated max, it increases the max as well. Shell uses the max/progress to display the progress % in the system notification, and need to handle the scenario where the max changes. The initial implementation would recalculate the progress, with makes it swing back and forth as dumpstate increases the max. This CL changes the Shell logic so the progress never go back, just forward. The drawback of this approach is that if dumpstate underestimate the maximum, the progress might get stuck in a high value (99%) early on, but such issue will be addressed in the long term by tuning the estimated max value. BUG: 26354314 Change-Id: I3a5416acaffaaa43fd28d2f1f8ec8ea12aa0d91e --- .../shell/BugreportProgressService.java | 82 ++++++++++++++----- .../android/shell/BugreportReceiverTest.java | 27 +++++- .../tests/src/com/android/shell/UiBot.java | 6 +- 3 files changed, 90 insertions(+), 25 deletions(-) diff --git a/packages/Shell/src/com/android/shell/BugreportProgressService.java b/packages/Shell/src/com/android/shell/BugreportProgressService.java index 7fae0ee6b6603..c9582ea1aa516 100644 --- a/packages/Shell/src/com/android/shell/BugreportProgressService.java +++ b/packages/Shell/src/com/android/shell/BugreportProgressService.java @@ -149,6 +149,10 @@ public class BugreportProgressService extends Service { // Passed to Message.obtain() when msg.arg2 is not used. private static final int UNUSED_ARG2 = -2; + // Maximum progress displayed (like 99.00%). + private static final int CAPPED_PROGRESS = 9900; + private static final int CAPPED_MAX = 10000; + /** * Delay before a screenshot is taken. *

@@ -427,7 +431,7 @@ public class BugreportProgressService extends Service { final NumberFormat nf = NumberFormat.getPercentInstance(); nf.setMinimumFractionDigits(2); nf.setMaximumFractionDigits(2); - final String percentText = nf.format((double) info.progress / info.max); + final String percentageText = nf.format((double) info.progress / info.max); final Action cancelAction = new Action.Builder(null, mContext.getString( com.android.internal.R.string.cancel), newCancelIntent(mContext, info)).build(); final Intent infoIntent = new Intent(mContext, BugreportProgressService.class); @@ -458,7 +462,7 @@ public class BugreportProgressService extends Service { .setContentTitle(title) .setTicker(title) .setContentText(name) - .setContentInfo(percentText) + .setContentInfo(percentageText) .setProgress(info.max, info.progress, false) .setOngoing(true) .setContentIntent(infoPendingIntent) @@ -472,7 +476,7 @@ public class BugreportProgressService extends Service { } if (DEBUG) { Log.d(TAG, "Sending 'Progress' notification for id " + info.id + "(pid " + info.pid - + "): " + percentText); + + "): " + percentageText); } NotificationManager.from(mContext).notify(TAG, info.id, notification); } @@ -545,25 +549,47 @@ public class BugreportProgressService extends Service { } activeProcesses++; final String progressKey = DUMPSTATE_PREFIX + pid + PROGRESS_SUFFIX; - final int progress = SystemProperties.getInt(progressKey, 0); - if (progress == 0) { + info.realProgress = SystemProperties.getInt(progressKey, 0); + if (info.realProgress == 0) { Log.v(TAG, "System property " + progressKey + " is not set yet"); } - final int max = SystemProperties.getInt(DUMPSTATE_PREFIX + pid + MAX_SUFFIX, 0); - final boolean maxChanged = max > 0 && max != info.max; - final boolean progressChanged = progress > 0 && progress != info.progress; + final String maxKey = DUMPSTATE_PREFIX + pid + MAX_SUFFIX; + info.realMax = SystemProperties.getInt(maxKey, info.max); + if (info.realMax <= 0 ) { + Log.w(TAG, "Property " + maxKey + " is not positive: " + info.max); + continue; + } + /* + * Checks whether the progress changed in a way that should be displayed to the user: + * - info.progress / info.max represents the displayed progress + * - info.realProgress / info.realMax represents the real progress + * - since the real progress can decrease, the displayed progress is only updated if it + * increases + * - the displayed progress is capped at a maximum (like 99%) + */ + final int oldPercentage = (CAPPED_MAX * info.progress) / info.max; + int newPercentage = (CAPPED_MAX * info.realProgress) / info.realMax; + int max = info.realMax; + int progress = info.realProgress; - if (progressChanged || maxChanged) { - if (progressChanged) { - if (DEBUG) Log.v(TAG, "Updating progress for PID " + pid + "(id: " + id - + ") from " + info.progress + " to " + progress); - info.progress = progress; - } - if (maxChanged) { - Log.i(TAG, "Updating max progress for PID " + pid + "(id: " + id - + ") from " + info.max + " to " + max); - info.max = max; + if (newPercentage > CAPPED_PROGRESS) { + progress = newPercentage = CAPPED_PROGRESS; + max = CAPPED_MAX; + } + + if (newPercentage > oldPercentage) { + if (DEBUG) { + if (progress != info.progress) { + Log.v(TAG, "Updating progress for PID " + pid + "(id: " + id + ") from " + + info.progress + " to " + progress); + } + if (max != info.max) { + Log.v(TAG, "Updating max progress for PID " + pid + "(id: " + id + ") from " + + info.max + " to " + max); + } } + info.progress = progress; + info.max = max; info.lastUpdate = System.currentTimeMillis(); updateProgress(info); } else { @@ -1450,15 +1476,25 @@ public class BugreportProgressService extends Service { String description; /** - * Maximum progress of the bugreport generation. + * Maximum progress of the bugreport generation as displayed by the UI. */ int max; /** - * Current progress of the bugreport generation. + * Current progress of the bugreport generation as displayed by the UI. */ int progress; + /** + * Maximum progress of the bugreport generation as reported by dumpstate. + */ + int realMax; + + /** + * Current progress of the bugreport generation as reported by dumpstate. + */ + int realProgress; + /** * Time of the last progress update. */ @@ -1568,10 +1604,12 @@ public class BugreportProgressService extends Service { @Override public String toString() { final float percent = ((float) progress * 100 / max); + final float realPercent = ((float) realProgress * 100 / realMax); return "id: " + id + ", pid: " + pid + ", name: " + name + ", finished: " + finished + "\n\ttitle: " + title + "\n\tdescription: " + description + "\n\tfile: " + bugreportFile + "\n\tscreenshots: " + screenshotFiles + "\n\tprogress: " + progress + "/" + max + " (" + percent + ")" + + "\n\treal progress: " + realProgress + "/" + realMax + " (" + realPercent + ")" + "\n\tlast_update: " + getFormattedLastUpdate() + "\naddingDetailsToZip: " + addingDetailsToZip + " addedDetailsToZip: " + addedDetailsToZip; @@ -1587,6 +1625,8 @@ public class BugreportProgressService extends Service { description = in.readString(); max = in.readInt(); progress = in.readInt(); + realMax = in.readInt(); + realProgress = in.readInt(); lastUpdate = in.readLong(); formattedLastUpdate = in.readString(); bugreportFile = readFile(in); @@ -1609,6 +1649,8 @@ public class BugreportProgressService extends Service { dest.writeString(description); dest.writeInt(max); dest.writeInt(progress); + dest.writeInt(realMax); + dest.writeInt(realProgress); dest.writeLong(lastUpdate); dest.writeString(getFormattedLastUpdate()); writeFile(dest, bugreportFile); diff --git a/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java b/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java index 17f6f6b5ac896..e1e0c3b448b75 100644 --- a/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java +++ b/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java @@ -26,6 +26,7 @@ import static com.android.shell.BugreportProgressService.EXTRA_PID; import static com.android.shell.BugreportProgressService.EXTRA_SCREENSHOT; import static com.android.shell.BugreportProgressService.INTENT_BUGREPORT_FINISHED; import static com.android.shell.BugreportProgressService.INTENT_BUGREPORT_STARTED; +import static com.android.shell.BugreportProgressService.POLLING_FREQUENCY; import static com.android.shell.BugreportProgressService.SCREENSHOT_DELAY_SECONDS; import java.io.BufferedOutputStream; @@ -92,7 +93,7 @@ public class BugreportReceiverTest extends InstrumentationTestCase { private static final String TAG = "BugreportReceiverTest"; // Timeout for UI operations, in milliseconds. - private static final int TIMEOUT = (int) BugreportProgressService.POLLING_FREQUENCY * 4; + private static final int TIMEOUT = (int) POLLING_FREQUENCY * 4; // Timeout for when waiting for a screenshot to finish. private static final int SAFE_SCREENSHOT_DELAY = SCREENSHOT_DELAY_SECONDS + 10; @@ -190,8 +191,30 @@ public class BugreportReceiverTest extends InstrumentationTestCase { SystemProperties.set(PROGRESS_PROPERTY, "500"); assertProgressNotification(NAME, nf.format(0.50)); + SystemProperties.set(PROGRESS_PROPERTY, "950"); + assertProgressNotification(NAME, nf.format(0.95)); + + // Make sure progress never goes back... SystemProperties.set(MAX_PROPERTY, "2000"); - assertProgressNotification(NAME, nf.format(0.25)); + Thread.sleep(POLLING_FREQUENCY + DateUtils.SECOND_IN_MILLIS); + assertProgressNotification(NAME, nf.format(0.95)); + + SystemProperties.set(PROGRESS_PROPERTY, "1000"); + assertProgressNotification(NAME, nf.format(0.95)); + + // ...only forward... + SystemProperties.set(PROGRESS_PROPERTY, "1902"); + assertProgressNotification(NAME, nf.format(0.9510)); + + SystemProperties.set(PROGRESS_PROPERTY, "1960"); + assertProgressNotification(NAME, nf.format(0.98)); + + // ...but never more than the capped value. + SystemProperties.set(PROGRESS_PROPERTY, "2000"); + assertProgressNotification(NAME, nf.format(0.99)); + + SystemProperties.set(PROGRESS_PROPERTY, "3000"); + assertProgressNotification(NAME, nf.format(0.99)); Bundle extras = sendBugreportFinishedAndGetSharedIntent(ID, mPlainTextPath, mScreenshotPath); diff --git a/packages/Shell/tests/src/com/android/shell/UiBot.java b/packages/Shell/tests/src/com/android/shell/UiBot.java index 384c3daa51c92..5bfe1a084bc4b 100644 --- a/packages/Shell/tests/src/com/android/shell/UiBot.java +++ b/packages/Shell/tests/src/com/android/shell/UiBot.java @@ -32,7 +32,7 @@ import static junit.framework.Assert.assertTrue; final class UiBot { private static final String TAG = "UiBot"; - private static final String SYSTEMUI_PACKAGED = "com.android.systemui"; + private static final String SYSTEMUI_PACKAGE = "com.android.systemui"; private final UiDevice mDevice; private final int mTimeout; @@ -51,8 +51,8 @@ final class UiBot { public UiObject getNotification(String text) { boolean opened = mDevice.openNotification(); Log.v(TAG, "openNotification(): " + opened); - boolean gotIt = mDevice.wait(Until.hasObject(By.pkg(SYSTEMUI_PACKAGED)), mTimeout); - assertTrue("could not get system ui (" + SYSTEMUI_PACKAGED + ")", gotIt); + boolean gotIt = mDevice.wait(Until.hasObject(By.pkg(SYSTEMUI_PACKAGE)), mTimeout); + assertTrue("could not get system ui (" + SYSTEMUI_PACKAGE + ")", gotIt); return getObject(text); }