Snap for 5622519 from 9b6f2643661e4667493debdcccbba3ec518eb963 to pi-platform-releasepie-platform-release

Change-Id: I8a8f866a21085bb92c452610b922e2651eb5ea15

4 files changed, 105 insertions, 63 deletions

diff --git a/src/com/android/documentsui/services/FileOperationService.java b/src/com/android/documentsui/services/FileOperationService.java
index 40f18d38a..fa97ebf17 100644
--- a/src/com/android/documentsui/services/FileOperationService.java
+++ b/src/com/android/documentsui/services/FileOperationService.java
@@ -37,13 +37,12 @@ import com.android.documentsui.base.Features;
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.util.ArrayList;
-import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
-import java.util.concurrent.atomic.AtomicReference;
 
 import javax.annotation.concurrent.GuardedBy;
 
@@ -99,9 +98,9 @@ public class FileOperationService extends Service implements Job.Listener {
 
     private static final int POOL_SIZE = 2;  // "pool size", not *max* "pool size".
 
-    private static final int NOTIFICATION_ID_PROGRESS = 0;
-    private static final int NOTIFICATION_ID_FAILURE = 1;
-    private static final int NOTIFICATION_ID_WARNING = 2;
+    @VisibleForTesting static final int NOTIFICATION_ID_PROGRESS = 1;
+    private static final int NOTIFICATION_ID_FAILURE = 2;
+    private static final int NOTIFICATION_ID_WARNING = 3;
 
     // The executor and job factory are visible for testing and non-final
     // so we'll have a way to inject test doubles from the test. It's
@@ -124,10 +123,11 @@ public class FileOperationService extends Service implements Job.Listener {
     @VisibleForTesting Features features;
 
     @GuardedBy("mJobs")
-    private final Map<String, JobRecord> mJobs = new HashMap<>();
+    private final Map<String, JobRecord> mJobs = new LinkedHashMap<>();
 
     // The job whose notification is used to keep the service in foreground mode.
-    private final AtomicReference<Job> mForegroundJob = new AtomicReference<>();
+    @GuardedBy("mJobs")
+    private Job mForegroundJob;
 
     private PowerManager mPowerManager;
     private PowerManager.WakeLock mWakeLock;  // the wake lock, if held.
@@ -270,6 +270,7 @@ public class FileOperationService extends Service implements Job.Listener {
             JobRecord record = mJobs.get(jobId);
             if (record != null) {
                 record.job.cancel();
+                updateForegroundState(record.job);
             }
         }
 
@@ -343,18 +344,23 @@ public class FileOperationService extends Service implements Job.Listener {
 
         Notification notification = job.getSetupNotification();
         // If there is no foreground job yet, set this job to foreground job.
-        if (mForegroundJob.compareAndSet(null, job)) {
-            if (DEBUG) Log.d(TAG, "Set foreground job to " + job.id);
-            foregroundManager.startForeground(NOTIFICATION_ID_PROGRESS, notification);
+        synchronized (mJobs) {
+            if (mForegroundJob == null) {
+                if (DEBUG) Log.d(TAG, "Set foreground job to " + job.id);
+                mForegroundJob = job;
+                foregroundManager.startForeground(NOTIFICATION_ID_PROGRESS, notification);
+            } else {
+                // Show start up notification
+                if (DEBUG) Log.d(TAG, "Posting notification for " + job.id);
+                notificationManager.notify(
+                        mForegroundJob == job ? null : job.id,
+                        NOTIFICATION_ID_PROGRESS,
+                        notification);
+            }
         }
 
-        // Show start up notification
-        if (DEBUG) Log.d(TAG, "Posting notification for " + job.id);
-        notificationManager.notify(
-                job.id, NOTIFICATION_ID_PROGRESS, notification);
-
         // Set up related monitor
-        JobMonitor monitor = new JobMonitor(job, notificationManager, handler, mJobs);
+        JobMonitor monitor = new JobMonitor(job);
         monitor.start();
     }
 
@@ -388,11 +394,12 @@ public class FileOperationService extends Service implements Job.Listener {
 
     @GuardedBy("mJobs")
     private void updateForegroundState(Job job) {
-        Job candidate = mJobs.isEmpty() ? null : mJobs.values().iterator().next().job;
+        Job candidate = getCandidateForegroundJob();
 
         // If foreground job is retiring and there is still work to do, we need to set it to a new
         // job.
-        if (mForegroundJob.compareAndSet(job, candidate)) {
+        if (mForegroundJob == job) {
+            mForegroundJob = candidate;
             if (candidate == null) {
                 if (DEBUG) Log.d(TAG, "Stop foreground");
                 // Remove the notification here just in case we're torn down before we have the
@@ -401,12 +408,11 @@ public class FileOperationService extends Service implements Job.Listener {
             } else {
                 if (DEBUG) Log.d(TAG, "Switch foreground job to " + candidate.id);
 
+                notificationManager.cancel(candidate.id, NOTIFICATION_ID_PROGRESS);
                 Notification notification = (candidate.getState() == Job.STATE_STARTED)
                         ? candidate.getSetupNotification()
                         : candidate.getProgressNotification();
-                foregroundManager.startForeground(NOTIFICATION_ID_PROGRESS, notification);
-                notificationManager.notify(candidate.id, NOTIFICATION_ID_PROGRESS,
-                        notification);
+                notificationManager.notify(NOTIFICATION_ID_PROGRESS, notification);
             }
         }
     }
@@ -435,6 +441,19 @@ public class FileOperationService extends Service implements Job.Listener {
         }
     }
 
+    @GuardedBy("mJobs")
+    private Job getCandidateForegroundJob() {
+        if (mJobs.isEmpty()) {
+            return null;
+        }
+        for (JobRecord rec : mJobs.values()) {
+            if (!rec.job.isFinished()) {
+                return rec.job;
+            }
+        }
+        return null;
+    }
+
     private static final class JobRecord {
         private final Job job;
         private final Future<?> future;
@@ -452,29 +471,22 @@ public class FileOperationService extends Service implements Job.Listener {
      * still need to update notifications if jobs hang, so instead of jobs pushing their states,
      * we poll states of jobs.
      */
-    private static final class JobMonitor implements Runnable {
+    private final class JobMonitor implements Runnable {
         private static final long PROGRESS_INTERVAL_MILLIS = 500L;
 
         private final Job mJob;
-        private final NotificationManager mNotificationManager;
-        private final Handler mHandler;
-        private final Object mJobsLock;
 
-        private JobMonitor(Job job, NotificationManager notificationManager, Handler handler,
-                Object jobsLock) {
+        private JobMonitor(Job job) {
             mJob = job;
-            mNotificationManager = notificationManager;
-            mHandler = handler;
-            mJobsLock = jobsLock;
         }
 
         private void start() {
-            mHandler.post(this);
+            handler.post(this);
         }
 
         @Override
         public void run() {
-            synchronized (mJobsLock) {
+            synchronized (mJobs) {
                 if (mJob.isFinished()) {
                     // Finish notification is already shown. Progress notification is removed.
                     // Just finish itself.
@@ -483,11 +495,13 @@ public class FileOperationService extends Service implements Job.Listener {
 
                 // Only job in set up state has progress bar
                 if (mJob.getState() == Job.STATE_SET_UP) {
-                    mNotificationManager.notify(
-                            mJob.id, NOTIFICATION_ID_PROGRESS, mJob.getProgressNotification());
+                    notificationManager.notify(
+                            mForegroundJob == mJob ? null : mJob.id,
+                            NOTIFICATION_ID_PROGRESS,
+                            mJob.getProgressNotification());
                 }
 
-                mHandler.postDelayed(this, PROGRESS_INTERVAL_MILLIS);
+                handler.postDelayed(this, PROGRESS_INTERVAL_MILLIS);
             }
         }
     }
diff --git a/tests/common/com/android/documentsui/services/TestForegroundManager.java b/tests/common/com/android/documentsui/services/TestForegroundManager.java
index f4ba1c191..8b226a0aa 100644
--- a/tests/common/com/android/documentsui/services/TestForegroundManager.java
+++ b/tests/common/com/android/documentsui/services/TestForegroundManager.java
@@ -23,17 +23,25 @@ import android.app.Notification;
 
 class TestForegroundManager implements FileOperationService.ForegroundManager {
 
+    private final TestNotificationManager mNotificationManager;
     private int mForegroundId = -1;
     private Notification mForegroundNotification;
 
+    TestForegroundManager(TestNotificationManager notificationManager) {
+        assert(notificationManager != null);
+        mNotificationManager = notificationManager;
+    }
+
     @Override
     public void startForeground(int id, Notification notification) {
         mForegroundId = id;
         mForegroundNotification = notification;
+        mNotificationManager.notify(null, mForegroundId, mForegroundNotification);
     }
 
     @Override
     public void stopForeground(boolean cancelNotification) {
+        mNotificationManager.cancel(null, mForegroundId);
         mForegroundId = -1;
         mForegroundNotification = null;
     }
@@ -45,12 +53,4 @@ class TestForegroundManager implements FileOperationService.ForegroundManager {
     void assertInBackground() {
         assertNull(mForegroundNotification);
     }
-
-    int getForegroundId() {
-        return mForegroundId;
-    }
-
-    Notification getForegroundNotification() {
-        return mForegroundNotification;
-    }
 }
diff --git a/tests/common/com/android/documentsui/services/TestNotificationManager.java b/tests/common/com/android/documentsui/services/TestNotificationManager.java
index 463f2d425..5adde6563 100644
--- a/tests/common/com/android/documentsui/services/TestNotificationManager.java
+++ b/tests/common/com/android/documentsui/services/TestNotificationManager.java
@@ -17,6 +17,7 @@
 package com.android.documentsui.services;
 
 import static junit.framework.Assert.assertEquals;
+import static junit.framework.Assert.assertFalse;
 import static junit.framework.Assert.assertNotNull;
 import static junit.framework.Assert.assertTrue;
 
@@ -32,21 +33,10 @@ import java.util.HashMap;
 
 class TestNotificationManager {
 
-    private final TestForegroundManager mForegroundManager;
     private final SparseArray<HashMap<String, Notification>> mNotifications = new SparseArray<>();
     private final Answer<Void> mAnswer = this::invoke;
 
-    TestNotificationManager(TestForegroundManager foregroundManager) {
-        assert(foregroundManager != null);
-        mForegroundManager = foregroundManager;
-    }
-
-    private void notify(String tag, int id, Notification notification) {
-        if (notification == mForegroundManager.getForegroundNotification()
-                && id != mForegroundManager.getForegroundId()) {
-            throw new IllegalStateException("Mismatching ID and notification.");
-        }
-
+    void notify(String tag, int id, Notification notification) {
         if (mNotifications.get(id) == null) {
             mNotifications.put(id, new HashMap<>());
         }
@@ -54,14 +44,10 @@ class TestNotificationManager {
         mNotifications.get(id).put(tag, notification);
     }
 
-    private void cancel(String tag, int id) {
+    void cancel(String tag, int id) {
         final HashMap<String, Notification> idMap = mNotifications.get(id);
         if (idMap != null && idMap.containsKey(tag)) {
-            final Notification notification = idMap.get(tag);
-            // Only cancel non-foreground notification
-            if (mForegroundManager.getForegroundNotification() != notification) {
-                idMap.remove(tag);
-            }
+            idMap.remove(tag);
         }
     }
 
@@ -88,6 +74,14 @@ class TestNotificationManager {
         return null;
     }
 
+    private boolean hasNotification(int id, String jobId) {
+        if (mNotifications.get(id) == null) {
+            return false;
+        }
+        Notification notification = mNotifications.get(id).get(jobId);
+        return notification != null;
+    }
+
     NotificationManager createNotificationManager() {
         return Mockito.mock(NotificationManager.class, mAnswer);
     }
@@ -100,4 +94,12 @@ class TestNotificationManager {
 
         assertEquals(expect, count);
     }
+
+    void assertHasNotification(int id, String jobid) {
+        assertTrue(hasNotification(id, jobid));
+    }
+
+    void assertNoNotification(int id, String jobid) {
+        assertFalse(hasNotification(id, jobid));
+    }
 }
diff --git a/tests/unit/com/android/documentsui/services/FileOperationServiceTest.java b/tests/unit/com/android/documentsui/services/FileOperationServiceTest.java
index a210c0ee1..fbaafe803 100644
--- a/tests/unit/com/android/documentsui/services/FileOperationServiceTest.java
+++ b/tests/unit/com/android/documentsui/services/FileOperationServiceTest.java
@@ -78,8 +78,8 @@ public class FileOperationServiceTest extends ServiceTestCase<FileOperationServi
         mExecutor = new TestScheduledExecutorService();
         mDeletionExecutor = new TestScheduledExecutorService();
         mHandler = new TestHandler();
-        mForegroundManager = new TestForegroundManager();
-        mTestNotificationManager = new TestNotificationManager(mForegroundManager);
+        mTestNotificationManager = new TestNotificationManager();
+        mForegroundManager = new TestForegroundManager(mTestNotificationManager);
         TestFeatures features = new TestFeatures();
         features.notificationChannel = InstrumentationRegistry.getTargetContext()
                 .getResources().getBoolean(R.bool.feature_notification_channel);
@@ -294,6 +294,32 @@ public class FileOperationServiceTest extends ServiceTestCase<FileOperationServi
         mTestNotificationManager.assertNumberOfNotifications(0);
     }
 
+    public void testNotificationUpdateAfterForegroundJobSwitch() throws Exception {
+        startService(createCopyIntent(newArrayList(ALPHA_DOC), BETA_DOC));
+        startService(createCopyIntent(newArrayList(GAMMA_DOC), DELTA_DOC));
+        Job job1 = mCopyJobs.get(0);
+        Job job2 = mCopyJobs.get(1);
+
+        mService.onStart(job1);
+        mTestNotificationManager.assertHasNotification(
+                FileOperationService.NOTIFICATION_ID_PROGRESS, null);
+
+        mService.onStart(job2);
+        mTestNotificationManager.assertHasNotification(
+                FileOperationService.NOTIFICATION_ID_PROGRESS, job2.id);
+
+        job1.cancel();
+        mService.onFinished(job1);
+        mTestNotificationManager.assertHasNotification(
+                FileOperationService.NOTIFICATION_ID_PROGRESS, null);
+        mTestNotificationManager.assertNoNotification(
+                FileOperationService.NOTIFICATION_ID_PROGRESS, job2.id);
+
+        job2.cancel();
+        mService.onFinished(job2);
+        mTestNotificationManager.assertNumberOfNotifications(0);
+    }
+
     private Intent createCopyIntent(ArrayList<DocumentInfo> files, DocumentInfo dest)
             throws Exception {
         DocumentStack stack = new DocumentStack();