Merge "New API: DumpableContainer.removeDumpable."

core/api/current.txt

@@ -46803,6 +46803,7 @@ package android.util {
 
   public interface DumpableContainer {
     method public boolean addDumpable(@NonNull android.util.Dumpable);
+    method public boolean removeDumpable(@NonNull android.util.Dumpable);
   }
 
   public class EventLog {

core/java/android/util/DumpableContainer.java

@@ -37,4 +37,15 @@ public interface DumpableContainer {
      * @return {@code true} if the dumpable was added, {@code false} if the call was ignored.
      */
     boolean addDumpable(@NonNull Dumpable dumpable);
+
+    /**
+     * Removes the given {@link Dumpable dumpable} from the container.
+     *
+     * @param dumpable dumpable to be removed.
+     *
+     * @return {@code true} if the dumpable was removed, {@code false} if it was not previously
+     * {@link #addDumpable(Dumpable) added} with the same identify (object reference) and
+     * {@link Dumpable#getDumpableName() name}.
+     */
+    boolean removeDumpable(@NonNull Dumpable dumpable);
 }

core/java/com/android/internal/util/dump/DumpableContainerImpl.java

@@ -24,11 +24,12 @@ import android.util.Log;
 import java.io.PrintWriter;
 import java.util.Objects;
 
-// TODO(b/149254050): add unit tests
 /**
  * Helper class for {@link DumpableContainer} implementations - they can "implement it by
  * association", i.e., by delegating the interface methods to a {@code DumpableContainerImpl}.
  *
+ * <p>This class is not thread safe.
+ *
  * @hide
  */
 public final class DumpableContainerImpl implements DumpableContainer {
@@ -58,6 +59,38 @@ public final class DumpableContainerImpl implements DumpableContainer {
         return true;
     }
 
+    @Override
+    public boolean removeDumpable(Dumpable dumpable) {
+        Objects.requireNonNull(dumpable, "dumpable");
+        String name = dumpable.getDumpableName();
+        if (name == null) {
+            if (DEBUG) {
+                Log.d(TAG, "Tried to remove nameless dumpable: " + dumpable);
+            }
+            return false;
+        }
+
+        Dumpable candidate = mDumpables.get(name);
+        if (candidate == null) {
+            if (DEBUG) {
+                Log.d(TAG, "Dumpable with name " + name + " not found");
+            }
+            return false;
+        }
+
+        // Make sure it's the right one
+        if (candidate != dumpable) {
+            Log.w(TAG, "removeDumpable(): passed dumpable (" + dumpable + ") named " + name
+                    + ", but internal dumpable with that name is " + candidate);
+            return false;
+        }
+        if (DEBUG) {
+            Log.d(TAG, "Removing dumpable named " + name);
+        }
+        mDumpables.remove(name);
+        return true;
+    }
+
     /**
      * Dumps the number of dumpable, without a newline.
      */

core/tests/coretests/src/com/android/internal/util/DumpableContainerImplTest.java

@@ -27,6 +27,7 @@ import org.junit.Test;
 
 import java.io.PrintWriter;
 import java.io.StringWriter;
+import java.util.concurrent.atomic.AtomicReference;
 
 public final class DumpableContainerImplTest {
 
@@ -41,7 +42,7 @@ public final class DumpableContainerImplTest {
 
     @Test
     public void testAddDumpable_dumpableWithoutName() {
-        Dumpable noNamer = new Dumpable() {
+        Dumpable namelessDumpable = new Dumpable() {
 
             @Override
             public String getDumpableName() {
@@ -54,7 +55,7 @@ public final class DumpableContainerImplTest {
             }
 
         };
-        assertThrows(NullPointerException.class, () -> mImpl.addDumpable(noNamer));
+        assertThrows(NullPointerException.class, () -> mImpl.addDumpable(namelessDumpable));
     }
 
     @Test
@@ -178,11 +179,76 @@ public final class DumpableContainerImplTest {
                         + "......6 Args: 4,8,15,16,23,42,\n");
     }
 
+    @Test
+    public void testRemoveDumpable_null() {
+        assertThrows(NullPointerException.class, () -> mImpl.removeDumpable(null));
+    }
+
+    @Test
+    public void testARemoveDumpable_dumpableWithoutName() {
+        // Need a non-null name initially otherwise it won't be added
+        AtomicReference<String> name = new AtomicReference<>("A Dumpable Has No Name");
+        Dumpable dumpable = new Dumpable() {
+
+            @Override
+            public String getDumpableName() {
+                return name.get();
+            }
+
+            @Override
+            public void dump(PrintWriter writer, String[] args) {
+                throw new UnsupportedOperationException("D'OH!");
+            }
+
+        };
+        assertWithMessage("addDumpable(with name)").that(mImpl.addDumpable(dumpable)).isTrue();
+
+        name.set(null);
+        assertWithMessage("removeDumpable(nameless)").that(mImpl.removeDumpable(dumpable))
+                .isFalse();
+    }
+
+    @Test
+    public void testRemoveDumpable_empty() {
+        CustomDumpable dumpable = new CustomDumpable("The name is Bond", "James Bond!");
+
+        assertWithMessage("removeDumpable()").that(mImpl.removeDumpable(dumpable)).isFalse();
+    }
+
+    @Test
+    public void testRemoveDumpable_sameNameButDifferentDumpable() {
+        CustomDumpable real = new CustomDumpable("Slim Shade", "Please stand up!");
+        CustomDumpable fake = new CustomDumpable("Slim Shade", "Please stand up!");
+
+        mImpl.addDumpable(real);
+
+        assertWithMessage("removeDumpable(fake)").that(mImpl.removeDumpable(fake)).isFalse();
+        assertWithMessage("removeDumpable(real)").that(mImpl.removeDumpable(real)).isTrue();
+    }
+
+    @Test
+    public void testRemoveDumpable_existing() {
+        CustomDumpable dumpable = new CustomDumpable("Homer", "D'ohmp!");
+
+        mImpl.addDumpable(dumpable);
+        mImpl.listDumpables("...", mWriter);
+        assertWithMessage("listDumpables()").that(getOutput()).isEqualTo("...1 dumpables: Homer\n");
+
+        assertWithMessage("removeDumpable()").that(mImpl.removeDumpable(dumpable)).isTrue();
+        resetOutput();
+        mImpl.listDumpables("...", mWriter);
+        assertWithMessage("listDumpables(...)").that(getOutput()).isEqualTo("...No dumpables\n");
+    }
+
     private String getOutput() {
         mSw.flush();
         return mSw.toString();
     }
 
+    private void resetOutput() {
+        mSw.getBuffer().setLength(0);
+    }
+
     private static final class CustomDumpable implements Dumpable {
         public final String name;
         public final String content;

packages/SystemUI/src/com/android/systemui/SystemUIApplication.java

@@ -272,7 +272,7 @@ public class SystemUIApplication extends Application implements
         mServicesStarted = true;
     }
 
-    // TODO(b/149254050): add unit tests? There doesn't seem to be a SystemUiApplicationTest...
+    // TODO(b/217567642): add unit tests? There doesn't seem to be a SystemUiApplicationTest...
     @Override
     public boolean addDumpable(Dumpable dumpable) {
         String name = dumpable.getDumpableName();
@@ -288,13 +288,21 @@ public class SystemUIApplication extends Application implements
         if (DEBUG) Log.d(TAG, "addDumpable(): adding '" + name + "' = " + dumpable);
         mDumpables.put(name, dumpable);
 
-        // TODO(b/149254050): replace com.android.systemui.dump.Dumpable by
+        // TODO(b/217567642): replace com.android.systemui.dump.Dumpable by
         // com.android.util.Dumpable and get rid of the intermediate lambda
         mDumpManager.registerDumpable(dumpable.getDumpableName(),
                 (fd, pw, args) -> dumpable.dump(pw, args));
         return true;
     }
 
+    // TODO(b/217567642): implement
+    @Override
+    public boolean removeDumpable(Dumpable dumpable) {
+        Log.w(TAG, "removeDumpable(" + dumpable + "): not implemented");
+
+        return false;
+    }
+
     @Override
     public void onConfigurationChanged(Configuration newConfig) {
         if (mServicesStarted) {