Merge "SdkMan2: Fix edge case when install/delete packages."

4 files changed, 120 insertions, 47 deletions

diff --git a/sdkmanager/libs/sdkuilib/src/com/android/sdkuilib/internal/repository/sdkman2/PackagesDiffLogic.java b/sdkmanager/libs/sdkuilib/src/com/android/sdkuilib/internal/repository/sdkman2/PackagesDiffLogic.java
index 871bf1b..bc3a3f4 100755
--- a/sdkmanager/libs/sdkuilib/src/com/android/sdkuilib/internal/repository/sdkman2/PackagesDiffLogic.java
+++ b/sdkmanager/libs/sdkuilib/src/com/android/sdkuilib/internal/repository/sdkman2/PackagesDiffLogic.java
@@ -23,11 +23,13 @@ import com.android.sdklib.internal.repository.PlatformPackage;
 import com.android.sdklib.internal.repository.PlatformToolPackage;
 import com.android.sdklib.internal.repository.SdkSource;
 import com.android.sdklib.internal.repository.ToolPackage;
+import com.android.sdklib.internal.repository.Package.UpdateInfo;
 import com.android.sdkuilib.internal.repository.UpdaterData;
 import com.android.sdkuilib.internal.repository.sdkman2.PkgItem.PkgState;
 
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.HashMap;
@@ -312,27 +314,34 @@ class PackagesDiffLogic {
 
         assert newPackages.size() == packages.length;
 
-        // Upgrade 'new' items to 'installed' for any local package we already know about
+        // Upgrade NEW items to INSTALLED for any local package we already know about.
+        // We can't just change the state of the NEW item to INSTALLED, we also need its
+        // installed package/archive information and so we swap them in-place in the items list.
+
         for (PkgCategory cat : op.getCategories()) {
             List<PkgItem> items = cat.getItems();
             for (int i = 0; i < items.size(); i++) {
                 PkgItem item = items.get(i);
 
-                if (item.hasUpdatePkg() && newPackages.contains(item.getUpdatePkg())) {
-                    // This item has an update package that is now installed.
-                    PkgItem installed = new PkgItem(item.getUpdatePkg(), PkgState.INSTALLED);
-                    unusedPackages.remove(item.getUpdatePkg());
-                    item.removeUpdate();
-                    items.add(installed);
-                    cat.setUnused(false);
-                    hasChanged = true;
+                if (item.hasUpdatePkg()) {
+                    Package newPkg = setContainsLocalPackage(newPackages, item.getUpdatePkg());
+                    if (newPkg != null) {
+                        // This item has an update package that is now installed.
+                        PkgItem installed = new PkgItem(newPkg, PkgState.INSTALLED);
+                        removePackageFromSet(unusedPackages, newPkg);
+                        item.removeUpdate();
+                        items.add(installed);
+                        cat.setUnused(false);
+                        hasChanged = true;
+                    }
                 }
 
-                if (newPackages.contains(item.getMainPackage())) {
-                    unusedPackages.remove(item.getMainPackage());
+                Package newPkg = setContainsLocalPackage(newPackages, item.getMainPackage());
+                if (newPkg != null) {
+                    removePackageFromSet(unusedPackages, newPkg);
                     if (item.getState() == PkgState.NEW) {
                         // This item has a main package that is now installed.
-                        item.setState(PkgState.INSTALLED);
+                        replace(items, i, new PkgItem(newPkg, PkgState.INSTALLED));
                         cat.setUnused(false);
                         hasChanged = true;
                     }
@@ -340,13 +349,18 @@ class PackagesDiffLogic {
             }
         }
 
-        // Downgrade 'installed' items to 'new' if their package isn't listed anymore
+        // Downgrade INSTALLED items to NEW if their package isn't listed anymore
         for (PkgCategory cat : op.getCategories()) {
-            for (PkgItem item : cat.getItems()) {
-                if (item.getState() == PkgState.INSTALLED &&
-                        !newPackages.contains(item.getMainPackage())) {
-                    item.setState(PkgState.NEW);
-                    hasChanged = true;
+            List<PkgItem> items = cat.getItems();
+            for (int i = 0; i < items.size(); i++) {
+                PkgItem item = items.get(i);
+
+                if (item.getState() == PkgState.INSTALLED) {
+                    Package newPkg = setContainsLocalPackage(newPackages, item.getMainPackage());
+                    if (newPkg == null) {
+                        replace(items, i, new PkgItem(item.getMainPackage(), PkgState.NEW));
+                        hasChanged = true;
+                    }
                 }
             }
         }
@@ -375,8 +389,72 @@ class PackagesDiffLogic {
         return hasChanged;
     }
 
-    /** Process all remote packages. Returns true if something changed.
-     * @param op */
+    /**
+     * Replaces the item at {@code index} in {@code list} with the new {@code obj} element.
+     * This uses {@link ArrayList#set(int, Object)} if possible, remove+add otherwise.
+     *
+     * @return The old item at the same index position.
+     * @throws IndexOutOfBoundsException if index out of range (index < 0 || index >= size()).
+     */
+    private <T> T replace(List<T> list, int index, T obj) {
+        if (list instanceof ArrayList<?>) {
+            return ((ArrayList<T>) list).set(index, obj);
+        } else {
+            T old = list.remove(index);
+            list.add(index, obj);
+            return old;
+        }
+    }
+
+    /**
+     * Checks whether the {@code newPackages} set contains a package that is the
+     * same as {@code pkgToFind}.
+     * This is based on Package being the same from an install point of view rather than
+     * pure object equality.
+     * @return The matching package from the {@code newPackages} set or null if not found.
+     */
+    private Package setContainsLocalPackage(Collection<Package> newPackages, Package pkgToFind) {
+        // Most of the time, local packages don't have the exact same hash code
+        // as new ones since the objects are similar but not exactly the same,
+        // for example their installed OS path cannot match (by definition) so
+        // their hash code do not match when used with Set.contains().
+
+        for (Package newPkg : newPackages) {
+            // Two packages are the same if they are compatible types,
+            // do not update each other and have the same revision number.
+            if (pkgToFind.canBeUpdatedBy(newPkg) == UpdateInfo.NOT_UPDATE &&
+                    pkgToFind.getRevision() == newPkg.getRevision()) {
+                return newPkg;
+            }
+        }
+
+        return null;
+    }
+
+    /**
+     * Removes the given package from the set.
+     * This is based on Package being the same from an install point of view rather than
+     * pure object equality.
+     */
+    private void removePackageFromSet(Collection<Package> packages, Package pkgToRemove) {
+        // First try to remove the package based on its hash code. This can fail
+        // for a variety of reasons, as explained in setContainsLocalPackage().
+        if (packages.remove(pkgToRemove)) {
+            return;
+        }
+
+        for (Package pkg : packages) {
+            // Two packages are the same if they are compatible types,
+            // or not updates of each other and have the same revision number.
+            if (pkgToRemove.canBeUpdatedBy(pkg) == UpdateInfo.NOT_UPDATE &&
+                    pkgToRemove.getRevision() == pkg.getRevision()) {
+                packages.remove(pkg);
+                return;
+            }
+        }
+    }
+
+    /** Process all remote packages. Returns true if something changed. */
     private boolean processSource(UpdateOp op, SdkSource source, Package[] packages) {
         boolean hasChanged = false;
         // Note: unusedPackages must respect the original packages order. It can't be a set.
@@ -396,22 +474,26 @@ class PackagesDiffLogic {
                 if (!(itemSource == source || (source != null && source.equals(itemSource)))) {
                     continue;
                 }
+
                 // Installed items have been dealt with the local source,
                 // so only change new items here
-                if (item.getState() == PkgState.NEW &&
-                        !newPackages.contains(item.getMainPackage())) {
-                    // This package is no longer part of the source.
-                    items.remove(i--);
-                    hasChanged = true;
-                    continue;
+                if (item.getState() == PkgState.NEW) {
+                    Package newPkg = setContainsLocalPackage(newPackages, item.getMainPackage());
+                    if (newPkg == null) {
+                        // This package is no longer part of the source.
+                        items.remove(i--);
+                        hasChanged = true;
+                        continue;
+                    }
                 }
 
                 cat.setUnused(false);
-                unusedPackages.remove(item.getMainPackage());
+                removePackageFromSet(unusedPackages, item.getMainPackage());
 
                 if (item.hasUpdatePkg()) {
-                    if (newPackages.contains(item.getUpdatePkg())) {
-                        unusedPackages.remove(item.getUpdatePkg());
+                    Package newPkg = setContainsLocalPackage(newPackages, item.getUpdatePkg());
+                    if (newPkg != null) {
+                        removePackageFromSet(unusedPackages, newPkg);
                     } else {
                         // This update is no longer part of the source
                         item.removeUpdate();
diff --git a/sdkmanager/libs/sdkuilib/src/com/android/sdkuilib/internal/repository/sdkman2/PackagesPage.java b/sdkmanager/libs/sdkuilib/src/com/android/sdkuilib/internal/repository/sdkman2/PackagesPage.java
index f703fcd..d5a6068 100755
--- a/sdkmanager/libs/sdkuilib/src/com/android/sdkuilib/internal/repository/sdkman2/PackagesPage.java
+++ b/sdkmanager/libs/sdkuilib/src/com/android/sdkuilib/internal/repository/sdkman2/PackagesPage.java
@@ -868,13 +868,12 @@ public class PackagesPage extends UpdaterPage
 
     /**
      * Indicate an install/delete operation is pending.
-     * This disable the install/delete buttons.
-     * Use {@link #endOperationPending()} to revert.
+     * This disables the install/delete buttons.
+     * Use {@link #endOperationPending()} to revert, typically in a {@code try..finally} block.
      */
     private void beginOperationPending() {
         mOperationPending = true;
-        mButtonInstall.setEnabled(false);
-        mButtonDelete.setEnabled(false);
+        updateButtonsState();
     }
 
     private void endOperationPending() {
@@ -883,10 +882,6 @@ public class PackagesPage extends UpdaterPage
     }
 
     private void updateButtonsState() {
-        if (mOperationPending) {
-            return;
-        }
-
         boolean canInstall = false;
         int numPackages = 0;
 
@@ -929,7 +924,7 @@ public class PackagesPage extends UpdaterPage
             }
         }
 
-        mButtonInstall.setEnabled(canInstall);
+        mButtonInstall.setEnabled(canInstall && !mOperationPending);
         mButtonInstall.setText(
                 numPackages == 0 ? "Install packages..." :
                     numPackages == 1 ? "Install 1 package..." :
@@ -952,7 +947,7 @@ public class PackagesPage extends UpdaterPage
             }
         }
 
-        mButtonDelete.setEnabled(canDelete);
+        mButtonDelete.setEnabled(canDelete && !mOperationPending);
         mButtonDelete.setText(
                 numPackages == 0 ? "Delete packages..." :
                     numPackages == 1 ? "Delete 1 package..." :
diff --git a/sdkmanager/libs/sdkuilib/src/com/android/sdkuilib/internal/repository/sdkman2/PkgItem.java b/sdkmanager/libs/sdkuilib/src/com/android/sdkuilib/internal/repository/sdkman2/PkgItem.java
index 5bc0b46..2675d8d 100755
--- a/sdkmanager/libs/sdkuilib/src/com/android/sdkuilib/internal/repository/sdkman2/PkgItem.java
+++ b/sdkmanager/libs/sdkuilib/src/com/android/sdkuilib/internal/repository/sdkman2/PkgItem.java
@@ -30,7 +30,7 @@ import com.android.sdklib.internal.repository.Package.UpdateInfo;
  * The state or update package can change later.
  */
 public class PkgItem implements Comparable<PkgItem> {
-    private PkgState mState;
+    private final PkgState mState;
     private final Package mMainPkg;
     private Package mUpdatePkg;
     private boolean mChecked;
@@ -102,10 +102,6 @@ public class PkgItem implements Comparable<PkgItem> {
         return mState;
     }
 
-    public void setState(PkgState state) {
-        mState = state;
-    }
-
     public SdkSource getSource() {
         return mMainPkg.getParentSource();
     }
diff --git a/sdkmanager/libs/sdkuilib/tests/com/android/sdkuilib/internal/repository/sdkman2/PackagesDiffLogicTest.java b/sdkmanager/libs/sdkuilib/tests/com/android/sdkuilib/internal/repository/sdkman2/PackagesDiffLogicTest.java
index 923a5fe..bbef6d3 100755
--- a/sdkmanager/libs/sdkuilib/tests/com/android/sdkuilib/internal/repository/sdkman2/PackagesDiffLogicTest.java
+++ b/sdkmanager/libs/sdkuilib/tests/com/android/sdkuilib/internal/repository/sdkman2/PackagesDiffLogicTest.java
@@ -182,9 +182,9 @@ public class PackagesDiffLogicTest extends TestCase {
                 "-- <INSTALLED, pkg:MockEmptyPackage 'type1' rev=1, updated by:MockEmptyPackage 'type1' rev=2>\n",
                 getTree(m, true /*displaySortByApi*/));
 
-        // Now simulate a reload that clears the package list and create similar
+        // Now simulate a reload that clears the package list and creates similar
         // objects but not the same references. The only difference is that updateXyz
-        // returns false since they don't change anything.
+        // returns false since nothing changes.
 
         m.updateStart();
         // First insert local packages
@@ -285,7 +285,7 @@ public class PackagesDiffLogicTest extends TestCase {
         m.updateStart();
         // No local packages
         assertTrue(m.updateSourcePackages(true /*sortByApi*/, null /*locals*/, new Package[0]));
-        assertTrue(m.updateSourcePackages(true /*sortByApi*/, src1, new Package[] {
+        assertFalse(m.updateSourcePackages(true /*sortByApi*/, src1, new Package[] {
                 new MockEmptyPackage(src1, "type1", 1)
         }));