From 51ff5c30f944623739a36e32f838aec52ede022c Mon Sep 17 00:00:00 2001 From: Daniele Fognini Date: Sat, 20 Jul 2019 14:41:12 +0200 Subject: [PATCH] fix upload list comparator to make it respect the comparator contract Signed-off-by: Daniele Fognini --- .../com/owncloud/android/db/OCUpload.java | 24 ++- .../owncloud/android/db/OCUploadComparator.kt | 75 +++++++ .../android/ui/adapter/UploadListAdapter.java | 45 +---- .../android/ui/db/OCUploadComparatorTest.kt | 189 ++++++++++++++++++ 4 files changed, 286 insertions(+), 47 deletions(-) create mode 100644 src/main/java/com/owncloud/android/db/OCUploadComparator.kt create mode 100644 src/test/java/com/owncloud/android/ui/db/OCUploadComparatorTest.kt diff --git a/src/main/java/com/owncloud/android/db/OCUpload.java b/src/main/java/com/owncloud/android/db/OCUpload.java index ee8d8b3c05..c5259307c9 100644 --- a/src/main/java/com/owncloud/android/db/OCUpload.java +++ b/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 */ diff --git a/src/main/java/com/owncloud/android/db/OCUploadComparator.kt b/src/main/java/com/owncloud/android/db/OCUploadComparator.kt new file mode 100644 index 0000000000..6ef5e63cc8 --- /dev/null +++ b/src/main/java/com/owncloud/android/db/OCUploadComparator.kt @@ -0,0 +1,75 @@ +/* + * NextCloud Android client application + * + * @copyright Copyright (C) 2019 Daniele Fognini + * + * 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 . + */ +package com.owncloud.android.db + +/** + * Sorts OCUpload by (uploadStatus, uploadingNow, uploadEndTimeStamp, uploadId). + */ +class OCUploadComparator : Comparator { + @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) + } +} diff --git a/src/main/java/com/owncloud/android/ui/adapter/UploadListAdapter.java b/src/main/java/com/owncloud/android/ui/adapter/UploadListAdapter.java index 409437867a..56252e7f6f 100755 --- a/src/main/java/com/owncloud/android/ui/adapter/UploadListAdapter.java +++ b/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; @@ -714,7 +714,7 @@ public class UploadListAdapter extends SectionedRecyclerViewAdapter comparator = new Comparator() { - @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()); - } - }; } } diff --git a/src/test/java/com/owncloud/android/ui/db/OCUploadComparatorTest.kt b/src/test/java/com/owncloud/android/ui/db/OCUploadComparatorTest.kt new file mode 100644 index 0000000000..e7aee8f446 --- /dev/null +++ b/src/test/java/com/owncloud/android/ui/db/OCUploadComparatorTest.kt @@ -0,0 +1,189 @@ +/* + * NextCloud Android client application + * + * @copyright Copyright (C) 2019 Daniele Fognini + * + * 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 . + */ +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(name = "failed") + val failedLater = mock(name = "failedLater") + val failedSameTimeOtherId = mock(name = "failedSameTimeOtherId") + val equalsNotSame = mock(name = "equalsNotSame") + val inProgress = mock(name = "InProgress") + val inProgressNow = mock(name = "inProgressNow") + + fun uploads(): Array { + 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> { + 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)) + } + } + } +}