Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TimSort, first fix values, then sort #2781

Merged
merged 4 commits into from
Jul 13, 2018
Merged

Conversation

tobiasKaminsky
Copy link
Member

TimSort is crashing because values (e.g. upload status) is changing while sorting.
Therefore the idea is to "fix" these values for sorting.
Drawback is that it can happen that sorting is already outdated, once sorting is done.
But we refresh after each upload and also a manual refresh can be triggered.

This is currently #1 bug on google play console.

For developing, I set each value of OCUpload to random and had a crash on nearly every sorting/refresh.
With this enhancement, I had no crash at all, although every value changed at any time.

We are using these fixedValues only for sorting and read-only.
So a changing status, e.g. because upload failed, will still be reflected on next sort.

Signed-off-by: tobiasKaminsky [email protected]

@codecov
Copy link

codecov bot commented Jul 2, 2018

Codecov Report

Merging #2781 into master will increase coverage by 0.02%.
The diff coverage is 31.85%.

@@            Coverage Diff            @@
##           master   #2781      +/-   ##
=========================================
+ Coverage    6.39%   6.42%   +0.02%     
=========================================
  Files         294     297       +3     
  Lines       29443   29721     +278     
  Branches     4252    4286      +34     
=========================================
+ Hits         1884    1909      +25     
- Misses      27272   27528     +256     
+ Partials      287     284       -3
Impacted Files Coverage Δ
...owncloud/android/ui/adapter/UploadListAdapter.java 0% <0%> (ø) ⬆️
.../owncloud/android/files/services/FileUploader.java 0.2% <0%> (ø) ⬆️
...cloud/android/datamodel/UploadsStorageManager.java 20.16% <100%> (ø) ⬆️
...ncloud/android/operations/UploadFileOperation.java 25.54% <100%> (ø) ⬆️
...rc/main/java/com/owncloud/android/db/OCUpload.java 29.62% <38.63%> (-2.12%) ⬇️
...ncloud/android/datamodel/SyncedFolderProvider.java 8.8% <0%> (-1.3%) ⬇️
...loud/android/datamodel/FileDataStorageManager.java 10.52% <0%> (-0.06%) ⬇️
...ncloud/android/ui/fragment/OCFileListFragment.java 0% <0%> (ø) ⬆️
.../java/com/owncloud/android/utils/DisplayUtils.java 5.72% <0%> (ø) ⬆️
...oud/android/ui/activity/NotificationsActivity.java 0% <0%> (ø) ⬆️
... and 33 more

@AndyScherzinger
Copy link
Member

AndyScherzinger commented Jul 2, 2018

👍 code changes look good to me, m-prefixing is fine to have the file consistent b/c a removal within the whole file would lead to a large amount of changes throughout the app which would pollute this PR review wise, but we might tackle this in a separate PR :)

Approved with PullApprove

@nextcloud nextcloud deleted a comment Jul 2, 2018
@nextcloud nextcloud deleted a comment from tobiasKaminsky Jul 2, 2018
@nextcloud nextcloud deleted a comment from AndyScherzinger Jul 2, 2018
@nextcloud nextcloud deleted a comment from tobiasKaminsky Jul 2, 2018
AndyScherzinger added a commit that referenced this pull request Jul 4, 2018
Backport of #2781: TimSort, first fix values, then sort
@mario
Copy link
Contributor

mario commented Jul 12, 2018

👍

Approved with PullApprove

@nextcloud nextcloud deleted a comment Jul 12, 2018
@nextcloud nextcloud deleted a comment Jul 12, 2018
@nextcloud nextcloud deleted a comment Jul 12, 2018
@nextcloud nextcloud deleted a comment Jul 12, 2018
@nextcloud nextcloud deleted a comment Jul 12, 2018
@nextcloud nextcloud deleted a comment Jul 12, 2018
@nextcloud nextcloud deleted a comment Jul 12, 2018
@nextcloud nextcloud deleted a comment Jul 12, 2018
@nextcloud nextcloud deleted a comment Jul 12, 2018
@nextcloud nextcloud deleted a comment Jul 12, 2018
@nextcloud nextcloud deleted a comment Jul 12, 2018
@nextcloud nextcloud deleted a comment Jul 12, 2018
Signed-off-by: tobiasKaminsky <[email protected]>
@nextcloud nextcloud deleted a comment Jul 13, 2018
@nextcloud nextcloud deleted a comment Jul 13, 2018
@nextcloud nextcloud deleted a comment Jul 13, 2018
@nextcloud nextcloud deleted a comment Jul 13, 2018
@nextcloud nextcloud deleted a comment Jul 13, 2018
@nextcloud nextcloud deleted a comment Jul 13, 2018
@nextcloud nextcloud deleted a comment Jul 13, 2018
Signed-off-by: tobiasKaminsky <[email protected]>
@nextcloud nextcloud deleted a comment Jul 13, 2018
@nextcloud nextcloud deleted a comment Jul 13, 2018
@nextcloud-android-bot
Copy link
Collaborator

Lint

This 93 warnings
Master 93 warnings

FindBugs (new)

Warning TypeNumber
Bad practice Warnings35
Correctness Warnings165
Experimental Warnings4
Internationalization Warnings12
Malicious code vulnerability Warnings9
Multithreaded correctness Warnings9
Performance Warnings159
Security Warnings163
Dodgy code Warnings207
Total763

FindBugs (master)

Warning TypeNumber
Bad practice Warnings35
Correctness Warnings166
Experimental Warnings4
Internationalization Warnings12
Malicious code vulnerability Warnings9
Multithreaded correctness Warnings9
Performance Warnings158
Security Warnings163
Dodgy code Warnings207
Total763

@tobiasKaminsky tobiasKaminsky merged commit 7da5203 into master Jul 13, 2018
@tobiasKaminsky tobiasKaminsky deleted the timSortWithFixedValues branch July 13, 2018 12:22
@AndyScherzinger AndyScherzinger added this to the Nextcloud App 3.3.0 milestone Jul 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants