From 079f89614c49364bb907783b008827fbc306dd73 Mon Sep 17 00:00:00 2001 From: Felipe Leme Date: Mon, 18 Apr 2016 12:04:23 -0700 Subject: [PATCH] Remove initial screenshot on interactive bugreport. One of the changes in the 'interactive bugreport' bugreport workflow introduced on N is that the initial screenshot was taken right away (by Shell, not dumpstate). Unfortunately, such initial screenshot is often delayed when the system is overload. Also, if the user is not interested in a screenshot, it would be adding more load on the system unnecessarily. Given these issues, and the fact that the user can still easily take an initial screenhsot by selecting the proper notification action, the initial screenshot is being removed. BUG: 28167977 Change-Id: I2cf6616ce3124102b62ec9a36dc5a0ce6455a909 --- .../shell/BugreportProgressService.java | 40 ++++++------------- .../android/shell/BugreportReceiverTest.java | 16 ++++---- 2 files changed, 22 insertions(+), 34 deletions(-) diff --git a/packages/Shell/src/com/android/shell/BugreportProgressService.java b/packages/Shell/src/com/android/shell/BugreportProgressService.java index 502eed139d174..346ae201cdb69 100644 --- a/packages/Shell/src/com/android/shell/BugreportProgressService.java +++ b/packages/Shell/src/com/android/shell/BugreportProgressService.java @@ -336,7 +336,7 @@ public class BugreportProgressService extends Service { launchBugreportInfoDialog(id); break; case INTENT_BUGREPORT_SCREENSHOT: - takeScreenshot(id, true); + takeScreenshot(id); break; case INTENT_BUGREPORT_SHARE: shareBugreport(id, (BugreportInfo) intent.getParcelableExtra(EXTRA_INFO)); @@ -417,8 +417,6 @@ public class BugreportProgressService extends Service { return true; } mProcesses.put(info.id, info); - // Take initial screenshot. - takeScreenshot(id, false); updateProgress(info); return true; } @@ -635,19 +633,11 @@ public class BugreportProgressService extends Service { /** * Starting point for taking a screenshot. *

- * If {@code delayed} is set, it first display a toast message and waits - * {@link #SCREENSHOT_DELAY_SECONDS} seconds before taking it, otherwise it takes the screenshot - * right away. - *

- * Typical usage is delaying when taken from the notification action, and taking it right away - * upon receiving a {@link #INTENT_BUGREPORT_STARTED}. + * It first display a toast message and waits {@link #SCREENSHOT_DELAY_SECONDS} seconds before + * taking the screenshot. */ - private void takeScreenshot(int id, boolean delayed) { - if (delayed) { - // Only logs screenshots requested from the notification action. - MetricsLogger.action(this, - MetricsEvent.ACTION_BUGREPORT_NOTIFICATION_ACTION_SCREENSHOT); - } + private void takeScreenshot(int id) { + MetricsLogger.action(this, MetricsEvent.ACTION_BUGREPORT_NOTIFICATION_ACTION_SCREENSHOT); if (getInfo(id) == null) { // Most likely am killed Shell before user tapped the notification. Since system might // be too busy anwyays, it's better to ignore the notification and switch back to the @@ -659,19 +649,15 @@ public class BugreportProgressService extends Service { return; } setTakingScreenshot(true); - if (delayed) { - collapseNotificationBar(); - final String msg = mContext.getResources() - .getQuantityString(com.android.internal.R.plurals.bugreport_countdown, - SCREENSHOT_DELAY_SECONDS, SCREENSHOT_DELAY_SECONDS); - Log.i(TAG, msg); - // Show a toast just once, otherwise it might be captured in the screenshot. - Toast.makeText(mContext, msg, Toast.LENGTH_SHORT).show(); + collapseNotificationBar(); + final String msg = mContext.getResources() + .getQuantityString(com.android.internal.R.plurals.bugreport_countdown, + SCREENSHOT_DELAY_SECONDS, SCREENSHOT_DELAY_SECONDS); + Log.i(TAG, msg); + // Show a toast just once, otherwise it might be captured in the screenshot. + Toast.makeText(mContext, msg, Toast.LENGTH_SHORT).show(); - takeScreenshot(id, SCREENSHOT_DELAY_SECONDS); - } else { - takeScreenshot(id, 0); - } + takeScreenshot(id, SCREENSHOT_DELAY_SECONDS); } /** diff --git a/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java b/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java index 537e4c53087c5..f76fb260a18ab 100644 --- a/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java +++ b/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java @@ -216,7 +216,7 @@ public class BugreportReceiverTest extends InstrumentationTestCase { Bundle extras = sendBugreportFinishedAndGetSharedIntent(ID, mPlainTextPath, mScreenshotPath); assertActionSendMultiple(extras, BUGREPORT_CONTENT, SCREENSHOT_CONTENT, ID, PID, ZIP_FILE, - NAME, NO_TITLE, NO_DESCRIPTION, 1, RENAMED_SCREENSHOTS); + NAME, NO_TITLE, NO_DESCRIPTION, 0, RENAMED_SCREENSHOTS); assertServiceNotRunning(); } @@ -266,7 +266,7 @@ public class BugreportReceiverTest extends InstrumentationTestCase { Bundle extras = acceptBugreportAndGetSharedIntent(ID); assertActionSendMultiple(extras, BUGREPORT_CONTENT, SCREENSHOT_CONTENT, ID, PID, ZIP_FILE, - NAME, NO_TITLE, NO_DESCRIPTION, 2, RENAMED_SCREENSHOTS); + NAME, NO_TITLE, NO_DESCRIPTION, 1, RENAMED_SCREENSHOTS); assertServiceNotRunning(); } @@ -283,6 +283,8 @@ public class BugreportReceiverTest extends InstrumentationTestCase { resetProperties(); sendBugreportStarted(1000); + waitForScreenshotButtonEnabled(true); + takeScreenshot(); sendBugreportFinished(ID, mPlainTextPath, NO_SCREENSHOT); waitShareNotification(ID); @@ -340,7 +342,7 @@ public class BugreportReceiverTest extends InstrumentationTestCase { Bundle extras = sendBugreportFinishedAndGetSharedIntent(ID, mPlainTextPath, mScreenshotPath); assertActionSendMultiple(extras, BUGREPORT_CONTENT, SCREENSHOT_CONTENT, ID, PID, TITLE, - NEW_NAME, TITLE, mDescription, 1, RENAMED_SCREENSHOTS); + NEW_NAME, TITLE, mDescription, 0, RENAMED_SCREENSHOTS); assertServiceNotRunning(); } @@ -377,7 +379,7 @@ public class BugreportReceiverTest extends InstrumentationTestCase { Bundle extras = sendBugreportFinishedAndGetSharedIntent(ID, plainText? mPlainTextPath : mZipPath, mScreenshotPath); assertActionSendMultiple(extras, BUGREPORT_CONTENT, SCREENSHOT_CONTENT, ID, PID, TITLE, - NEW_NAME, TITLE, mDescription, 1, RENAMED_SCREENSHOTS); + NEW_NAME, TITLE, mDescription, 0, RENAMED_SCREENSHOTS); assertServiceNotRunning(); } @@ -404,7 +406,7 @@ public class BugreportReceiverTest extends InstrumentationTestCase { Bundle extras = sendBugreportFinishedAndGetSharedIntent(ID, mZipPath, mScreenshotPath); assertActionSendMultiple(extras, BUGREPORT_CONTENT, SCREENSHOT_CONTENT, ID, PID, ZIP_FILE, - NO_NAME, NO_TITLE, mDescription, 1, DIDNT_RENAME_SCREENSHOTS); + NO_NAME, NO_TITLE, mDescription, 0, DIDNT_RENAME_SCREENSHOTS); assertServiceNotRunning(); } @@ -449,7 +451,7 @@ public class BugreportReceiverTest extends InstrumentationTestCase { // title.txt and description.txt entries. extras = sendBugreportFinishedAndGetSharedIntent(ID2, mZipPath2, NO_SCREENSHOT); assertActionSendMultiple(extras, BUGREPORT_CONTENT, NO_SCREENSHOT, ID2, PID2, TITLE2, - NEW_NAME2, TITLE2, DESCRIPTION2, 1, RENAMED_SCREENSHOTS); + NEW_NAME2, TITLE2, DESCRIPTION2, 0, RENAMED_SCREENSHOTS); assertServiceNotRunning(); } @@ -500,7 +502,7 @@ public class BugreportReceiverTest extends InstrumentationTestCase { // Finally, share bugreport. Bundle extras = acceptBugreportAndGetSharedIntent(ID); assertActionSendMultiple(extras, BUGREPORT_CONTENT, SCREENSHOT_CONTENT, ID, PID, TITLE, - NAME, TITLE, mDescription, 1, RENAMED_SCREENSHOTS); + NAME, TITLE, mDescription, 0, RENAMED_SCREENSHOTS); assertServiceNotRunning(); }