Fix upload list comparator to make it respect the comparator contract

[fogninid]

the current comparator implementation is correct only if there is only one "isUpdating" file in the array.
This can happen for example as a result of #4244, or if concurrent uploads will be impelmented.

Change the implementation to make it follow the simpler Comparator.thenComparing pattern

[ezaquarii]

While being here, I'm sure we can apply Scout's Rule #1 - leave the place cleaner than we found it. ;)

Also, if possible, could we write the test in Kotlin? The goal is to accelerate and encourage it's adoption, as Kotlin is de facto primary development language for Android now.

[codecov]

Codecov Report

Merging #4245 into master will increase coverage by 0.05%.
The diff coverage is 62.06%.

@@             Coverage Diff              @@
##             master    #4245      +/-   ##
============================================
+ Coverage     15.99%   16.05%   +0.05%     
  Complexity        1        1              
============================================
  Files           340      341       +1     
  Lines         31155    31158       +3     
  Branches       4424     4421       -3     
============================================
+ Hits           4983     5002      +19     
+ Misses        25313    25295      -18     
- Partials        859      861       +2

Impacted Files	Coverage Δ	Complexity Δ
...rc/main/java/com/owncloud/android/db/OCUpload.java	34.23% <0%> (ø)	0 <0> (ø)	⬇️
...owncloud/android/ui/adapter/UploadListAdapter.java	0% <0%> (ø)	0 <0> (ø)	⬇️
...java/com/owncloud/android/db/OCUploadComparator.kt	75% <75%> (ø)	0 <0> (?)
.../java/com/owncloud/android/utils/DisplayUtils.java	22.97% <0%> (-1.28%)	0% <0%> (ø)
...in/java/com/owncloud/android/datamodel/OCFile.java	64.67% <0%> (-0.92%)	0% <0%> (ø)
...loud/android/datamodel/ThumbnailsCacheManager.java	35.66% <0%> (-0.56%)	0% <0%> (ø)
...ncloud/android/ui/fragment/OCFileListFragment.java	24.73% <0%> (+0.26%)	0% <0%> (ø)	⬇️
...owncloud/android/ui/adapter/OCFileListAdapter.java	31.15% <0%> (+0.63%)	0% <0%> (ø)	⬇️
...va/com/owncloud/android/ui/SquareLinearLayout.java	50% <0%> (+50%)	0% <0%> (ø)	⬇️

[nextcloud-android-bot]

IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/10057

[nextcloud-android-bot]

IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/10059

[ezaquarii]

Yes this is possible that nobody hit that path on an old phone. The app will compile without trouble, but it will fail at runtime. @tobiasKaminsky I believe the app is tested on API 14. Is it possible that test is not covering this path?

…

On July 25, 2019 9:38:18 PM UTC, Daniele Fognini ***@***.***> wrote: fogninid commented on this pull request. > @@ -681,6 +682,72 @@ private void openFileWithDefault(String localPath) { CURRENT, FINISHED, FAILED } + /** + * sort uploads by (status, uploadingNow, updateTime, uploadId). + * + * Lower language level version of: + * + * Comparator.nullsFirst( + * Comparator.comparing(OCUpload::getFixedUploadStatus) + * .thenComparing(Comparator.comparing(OCUpload::isFixedUploadingNow).reversed()) + * .thenComparing(Comparator.comparing(OCUpload::getFixedUploadEndTimeStamp).reversed()) + * .thenComparing(OCUpload::getFixedUploadId)) + * + */ + @VisibleForTesting + static Comparator<OCUpload> uploadListComparator = new Comparator<OCUpload>() { My question why is it working now! It was changed in master 6 months ago: cb71348#diff-24801376b7c7edeb4e3089df542a4de8R676 I assume that nobody is actually using such old devices anymore or caring to update them. I will change it to use the older version, but you should probably consider to drop support or at least try to compile with such old sdk versions. -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #4245 (comment)

[nextcloud-android-bot]

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/10214.apk



To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

[nextcloud-android-bot]

Codacy

281

Lint

Type	master	PR
Warnings	59	59
Errors	0	0

SpotBugs (new)

Warning Type	Number
Bad practice Warnings	25
Correctness Warnings	70
Internationalization Warnings	12
Malicious code vulnerability Warnings	4
Multithreaded correctness Warnings	9
Performance Warnings	120
Security Warnings	47
Dodgy code Warnings	136
Total	423

SpotBugs (master)

Warning Type	Number
Bad practice Warnings	25
Correctness Warnings	70
Internationalization Warnings	12
Malicious code vulnerability Warnings	4
Multithreaded correctness Warnings	9
Performance Warnings	120
Security Warnings	47
Dodgy code Warnings	136
Total	423

[fogninid]

@nextcloud-android-bot well done for deleting the unresolved conversation /s

I added the @Suppress("ReturnCount") because I understand that kotlin.Comparator is just a typealias for the java class, so Comparator.nullsFirst(...) would crash at Runtime because java on android does not have it for old API levels

[ezaquarii]

I added the @Suppress("ReturnCount") because I understand that kotlin.Comparator is just a typealias

Ok, let's go with the old impl and not spend more time massaging it into unreachable perfection.

Thanks. Awesome work with valuable analysis as a byproduct.

[tobiasKaminsky]

Thanks for your awesome work!