Merge pull request #4245 from fogninid/fixUploadListComparator

Fix upload list comparator to make it respect the comparator contract

src/main/java/com/owncloud/android/db/OCUpload.java

@@ -124,10 +124,10 @@ public class OCUpload implements Parcelable {
     /**
      * temporary values, used for sorting
      */
-    @Getter private UploadStatus fixedUploadStatus;
-    @Getter private boolean fixedUploadingNow;
-    @Getter private long fixedUploadEndTimeStamp;
-    @Getter private long fixedUploadId;
+    private UploadStatus fixedUploadStatus;
+    private boolean fixedUploadingNow;
+    private long fixedUploadEndTimeStamp;
+    private long fixedUploadId;
 
     /**
      * Main constructor.
@@ -206,6 +206,22 @@ public class OCUpload implements Parcelable {
         this.lastResult = lastResult != null ? lastResult : UploadResult.UNKNOWN;
     }
 
+    public UploadStatus getFixedUploadStatus() {
+        return fixedUploadStatus;
+    }
+
+    public boolean isFixedUploadingNow() {
+        return fixedUploadingNow;
+    }
+
+    public long getFixedUploadEndTimeStamp() {
+        return fixedUploadEndTimeStamp;
+    }
+
+    public long getFixedUploadId() {
+        return fixedUploadId;
+    }
+
     /**
      * @return the mimeType
      */

src/main/java/com/owncloud/android/db/OCUploadComparator.kt

@@ -0,0 +1,75 @@
+/*
+ * NextCloud Android client application
+ *
+ * @copyright Copyright (C) 2019 Daniele Fognini <dfogni@gmail.com>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <https://www.gnu.org/licenses/>.
+ */
+package com.owncloud.android.db
+
+/**
+ *  Sorts OCUpload by (uploadStatus, uploadingNow, uploadEndTimeStamp, uploadId).
+ */
+class OCUploadComparator : Comparator<OCUpload?> {
+    @Suppress("ReturnCount")
+    override fun compare(upload1: OCUpload?, upload2: OCUpload?): Int {
+        if (upload1 == null && upload2 == null) {
+            return 0
+        }
+        if (upload1 == null) {
+            return -1
+        }
+        if (upload2 == null) {
+            return 1
+        }
+
+        val compareUploadStatus = compareUploadStatus(upload1, upload2)
+        if (compareUploadStatus != 0) {
+            return compareUploadStatus
+        }
+
+        val compareUploadingNow = compareUploadingNow(upload1, upload2)
+        if (compareUploadingNow != 0) {
+            return compareUploadingNow
+        }
+
+        val compareUpdateTime = compareUpdateTime(upload1, upload2)
+        if (compareUpdateTime != 0) {
+            return compareUpdateTime
+        }
+
+        val compareUploadId = compareUploadId(upload1, upload2)
+        if (compareUploadId != 0) {
+            return compareUploadId
+        }
+
+        return 0
+    }
+
+    private fun compareUploadStatus(upload1: OCUpload, upload2: OCUpload): Int {
+        return upload1.fixedUploadStatus.compareTo(upload2.fixedUploadStatus)
+    }
+
+    private fun compareUploadingNow(upload1: OCUpload, upload2: OCUpload): Int {
+        return upload2.isFixedUploadingNow.compareTo(upload1.isFixedUploadingNow)
+    }
+
+    private fun compareUpdateTime(upload1: OCUpload, upload2: OCUpload): Int {
+        return upload2.fixedUploadEndTimeStamp.compareTo(upload1.fixedUploadEndTimeStamp)
+    }
+
+    private fun compareUploadId(upload1: OCUpload, upload2: OCUpload): Int {
+        return upload1.fixedUploadId.compareTo(upload2.fixedUploadId)
+    }
+}

src/main/java/com/owncloud/android/ui/adapter/UploadListAdapter.java

@@ -49,6 +49,7 @@ import com.owncloud.android.datamodel.ThumbnailsCacheManager;
 import com.owncloud.android.datamodel.UploadsStorageManager;
 import com.owncloud.android.datamodel.UploadsStorageManager.UploadStatus;
 import com.owncloud.android.db.OCUpload;
+import com.owncloud.android.db.OCUploadComparator;
 import com.owncloud.android.db.UploadResult;
 import com.owncloud.android.files.services.FileUploader;
 import com.owncloud.android.lib.common.utils.Log_OC;
@@ -59,7 +60,6 @@ import com.owncloud.android.utils.ThemeUtils;
 
 import java.io.File;
 import java.util.Arrays;
-import java.util.Comparator;
 
 import androidx.annotation.NonNull;
 import butterknife.BindView;
@@ -726,7 +726,7 @@ public class UploadListAdapter extends SectionedRecyclerViewAdapter<SectionedVie
             for (OCUpload upload : array) {
                 upload.setDataFixed(binder);
             }
-            Arrays.sort(array, comparator);
+            Arrays.sort(array, new OCUploadComparator());
 
             setItems(array);
         }
@@ -734,46 +734,5 @@ public class UploadListAdapter extends SectionedRecyclerViewAdapter<SectionedVie
         private int getGroupItemCount() {
             return items == null ? 0 : items.length;
         }
-
-        Comparator<OCUpload> comparator = new Comparator<OCUpload>() {
-            @Override
-            public int compare(OCUpload upload1, OCUpload upload2) {
-                if (upload1 == null && upload2 == null) {
-                    return 0;
-                }
-                if (upload1 == null) {
-                    return -1;
-                }
-                if (upload2 == null) {
-                    return 1;
-                }
-                if (UploadStatus.UPLOAD_IN_PROGRESS == upload1.getFixedUploadStatus()) {
-                    if (UploadStatus.UPLOAD_IN_PROGRESS != upload2.getFixedUploadStatus()) {
-                        return -1;
-                    }
-                    // both are in progress
-                    if (upload1.isFixedUploadingNow()) {
-                        return -1;
-                    } else if (upload2.isFixedUploadingNow()) {
-                        return 1;
-                    }
-                } else if (upload2.getFixedUploadStatus() == UploadStatus.UPLOAD_IN_PROGRESS) {
-                    return 1;
-                }
-                if (upload1.getFixedUploadEndTimeStamp() == 0 || upload2.getFixedUploadEndTimeStamp() == 0) {
-                    return compareUploadId(upload1, upload2);
-                } else {
-                    return compareUpdateTime(upload1, upload2);
-                }
-            }
-
-            private int compareUploadId(OCUpload upload1, OCUpload upload2) {
-                return Long.compare(upload1.getFixedUploadId(), upload2.getFixedUploadId());
-            }
-
-            private int compareUpdateTime(OCUpload upload1, OCUpload upload2) {
-                return Long.compare(upload2.getFixedUploadEndTimeStamp(), upload1.getFixedUploadEndTimeStamp());
-            }
-        };
     }
 }

src/test/java/com/owncloud/android/ui/db/OCUploadComparatorTest.kt

@@ -0,0 +1,189 @@
+/*
+ * NextCloud Android client application
+ *
+ * @copyright Copyright (C) 2019 Daniele Fognini <dfogni@gmail.com>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <https://www.gnu.org/licenses/>.
+ */
+package com.owncloud.android.ui.db
+
+import com.nhaarman.mockitokotlin2.mock
+import com.nhaarman.mockitokotlin2.whenever
+import com.owncloud.android.datamodel.UploadsStorageManager.UploadStatus.UPLOAD_FAILED
+import com.owncloud.android.datamodel.UploadsStorageManager.UploadStatus.UPLOAD_IN_PROGRESS
+import com.owncloud.android.db.OCUpload
+import com.owncloud.android.db.OCUploadComparator
+import org.junit.Assert.assertArrayEquals
+import org.junit.Assert.assertEquals
+import org.junit.BeforeClass
+import org.junit.Test
+import org.junit.runner.RunWith
+import org.junit.runners.Parameterized
+import org.junit.runners.Suite
+import org.mockito.MockitoAnnotations
+
+@RunWith(Suite::class)
+@Suite.SuiteClasses(
+    OCUploadComparatorTest.Ordering::class,
+    OCUploadComparatorTest.ComparatorContract::class
+)
+class OCUploadComparatorTest {
+
+    internal abstract class Base {
+        companion object {
+            val failed = mock<OCUpload>(name = "failed")
+            val failedLater = mock<OCUpload>(name = "failedLater")
+            val failedSameTimeOtherId = mock<OCUpload>(name = "failedSameTimeOtherId")
+            val equalsNotSame = mock<OCUpload>(name = "equalsNotSame")
+            val inProgress = mock<OCUpload>(name = "InProgress")
+            val inProgressNow = mock<OCUpload>(name = "inProgressNow")
+
+            fun uploads(): Array<OCUpload> {
+                return arrayOf(failed, failedLater, inProgress, inProgressNow, failedSameTimeOtherId)
+            }
+
+            @JvmStatic
+            @BeforeClass
+            fun setupMocks() {
+                MockitoAnnotations.initMocks(this)
+
+                whenever(failed.fixedUploadStatus).thenReturn(UPLOAD_FAILED)
+                whenever(inProgress.fixedUploadStatus).thenReturn(UPLOAD_IN_PROGRESS)
+                whenever(inProgressNow.fixedUploadStatus).thenReturn(UPLOAD_IN_PROGRESS)
+                whenever(failedLater.fixedUploadStatus).thenReturn(UPLOAD_FAILED)
+                whenever(failedSameTimeOtherId.fixedUploadStatus).thenReturn(UPLOAD_FAILED)
+                whenever(equalsNotSame.fixedUploadStatus).thenReturn(UPLOAD_FAILED)
+
+                whenever(inProgressNow.isFixedUploadingNow).thenReturn(true)
+                whenever(inProgress.isFixedUploadingNow).thenReturn(false)
+
+                whenever(failed.fixedUploadEndTimeStamp).thenReturn(42)
+                whenever(failedLater.fixedUploadEndTimeStamp).thenReturn(43)
+                whenever(failedSameTimeOtherId.fixedUploadEndTimeStamp).thenReturn(42)
+                whenever(equalsNotSame.fixedUploadEndTimeStamp).thenReturn(42)
+
+                whenever(failedLater.uploadId).thenReturn(43)
+                whenever(failedSameTimeOtherId.uploadId).thenReturn(40)
+                whenever(equalsNotSame.uploadId).thenReturn(40)
+            }
+        }
+    }
+
+    internal class Ordering : Base() {
+
+        @Test
+        fun `same are compared equals in the list`() {
+            assertEquals(0, OCUploadComparator().compare(failed, failed))
+        }
+
+        @Test
+        fun `in progress is before failed in the list`() {
+            assertEquals(1, OCUploadComparator().compare(failed, inProgress))
+        }
+
+        @Test
+        fun `in progress uploading now is before in progress in the list`() {
+            assertEquals(1, OCUploadComparator().compare(inProgress, inProgressNow))
+        }
+
+        @Test
+        fun `later upload end is earlier in the list`() {
+            assertEquals(1, OCUploadComparator().compare(failed, failedLater))
+        }
+
+        @Test
+        fun `smaller upload id is earlier in the list`() {
+            assertEquals(1, OCUploadComparator().compare(failed, failedLater))
+        }
+
+        @Test
+        fun `same parameters compare equal in the list`() {
+            assertEquals(0, OCUploadComparator().compare(failedSameTimeOtherId, equalsNotSame))
+        }
+
+        @Test
+        fun `sort some uploads in the list`() {
+            val array = arrayOf(
+                inProgress,
+                inProgressNow,
+                failedSameTimeOtherId,
+                inProgressNow,
+                null,
+                failedLater,
+                failed
+            )
+
+            array.sortWith(OCUploadComparator())
+
+            assertArrayEquals(
+                arrayOf(
+                    null,
+                    inProgressNow,
+                    inProgressNow,
+                    inProgress,
+                    failedLater,
+                    failedSameTimeOtherId,
+                    failed
+                ),
+                array
+            )
+        }
+    }
+
+    @RunWith(Parameterized::class)
+    internal class ComparatorContract(
+        private val upload1: OCUpload,
+        private val upload2: OCUpload,
+        private val upload3: OCUpload
+    ) : Base() {
+        companion object {
+            @JvmStatic
+            @Parameterized.Parameters(name = "{0}, {1}, {2}")
+            fun data(): List<Array<OCUpload>> {
+                return uploads().flatMap { u1 ->
+                    uploads().flatMap { u2 ->
+                        uploads().map { u3 ->
+                            arrayOf(u1, u2, u3)
+                        }
+                    }
+                }
+            }
+        }
+
+        @Test
+        fun `comparator is reflective`() {
+            assertEquals(
+                -OCUploadComparator().compare(upload1, upload2),
+                OCUploadComparator().compare(upload2, upload1)
+            )
+        }
+
+        @Test
+        fun `comparator is compatible with equals`() {
+            if (upload1 == upload2) {
+                assertEquals(0, OCUploadComparator().compare(upload1, upload2))
+            }
+        }
+
+        @Test
+        fun `comparator is transitive`() {
+            val compare12 = OCUploadComparator().compare(upload1, upload2)
+            val compare23 = OCUploadComparator().compare(upload2, upload3)
+
+            if (compare12 == compare23) {
+                assertEquals(compare12, OCUploadComparator().compare(upload1, upload3))
+            }
+        }
+    }
+}