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.

Sign up for GitHub

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

Jump to bottom

Do not run insertAllDBEntriesForSyncedFolder directly #5885

Merged
merged 1 commit into from
Apr 16, 2020
Merged

Do not run insertAllDBEntriesForSyncedFolder directly #5885

merged 1 commit into from
Apr 16, 2020

Conversation

tobiasKaminsky
Copy link
Member

@tobiasKaminsky tobiasKaminsky commented Apr 16, 2020 •
edited

Fix #5882

Previously we ran insertAllDBEntriesForSyncedFolder on UI, which causes a lag on very big folders.
Now we use FilesSyncJob, which does the same, but is a Job, which runs in background.

@ezaquarii @AndyScherzinger

Signed-off-by: tobiasKaminsky tobias@kaminsky.me

Testing

Writing tests is very important. Please try to write some tests for your PR.
If you need help, please do not hesitate to ask in this PR for help.

unit tests
instrumented tests
UI tests

  • Tests written, or not not needed

@tobiasKaminsky
Copy link
Member Author

/backport to stable-3.11

@tobiasKaminsky tobiasKaminsky mentioned this pull request Apr 16, 2020
Closed
@nextcloud-android-bot
Copy link
Collaborator

Unit test failed, but no output was generated. Maybe a preliminary stage failed.

@nextcloud-android-bot
Copy link
Collaborator

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

@ezaquarii
Copy link
Collaborator

@tobiasKaminsky This will conflict with #5879 where FileSyncJob has been removed.

@tobiasKaminsky
Copy link
Member Author

@tobiasKaminsky This will conflict with #5879 where FileSyncJob has been removed.

True.
Then I will base this onto stable-3.11 only.
Can you make sure that #5879 behaves the same (no lagging UI on saving)?

@tobiasKaminsky tobiasKaminsky changed the base branch from master to stable-3.11 April 16, 2020 09:57
@tobiasKaminsky tobiasKaminsky changed the base branch from stable-3.11 to master April 16, 2020 09:58
@tobiasKaminsky tobiasKaminsky changed the base branch from master to stable-3.11 April 16, 2020 09:58
@tobiasKaminsky tobiasKaminsky added this to the Nextcloud App 3.11.1 milestone Apr 16, 2020
@nextcloud-android-bot
Copy link
Collaborator

Codacy Here is an overview of what got changed by this pull request:

Complexity decreasing per file
==============================
+ src/main/java/com/owncloud/android/utils/FilesSyncHelper.java  -1

See the complete overview on Codacy

@ezaquarii
Copy link
Collaborator

Then I will base this onto stable-3.11 only.
Can you make sure that #5879 behaves the same (no lagging UI on saving)?

@tobiasKaminsky This patch cannot be applied there as we're using BackgroundJobManager API instead of FilesSyncHelper. Semantics seem to be identical, so I'll make analoguous change in my branch using new API.

ezaquarii
ezaquarii approved these changes Apr 16, 2020
View reviewed changes
Copy link
Collaborator

@ezaquarii ezaquarii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

ezaquarii
ezaquarii reviewed Apr 16, 2020
View reviewed changes
src/main/java/com/owncloud/android/MainApp.java
}
}

FilesSyncHelper.startFilesSyncJobNow(null);
preferences.setAutoUploadInit(true);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tobiasKaminsky I'm not sure what this flag is supposed to do in this context?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No idea neither, but it is just c&p, so should not be problematic…

@nextcloud-android-bot
Copy link
Collaborator

Lint

Typestable-3.11PR
Warnings7777
Errors00

SpotBugs (new)

Warning TypeNumber
Bad practice Warnings26
Correctness Warnings70
Internationalization Warnings13
Malicious code vulnerability Warnings5
Multithreaded correctness Warnings9
Performance Warnings77
Security Warnings45
Dodgy code Warnings139
Total384

SpotBugs (stable-3.11)

Warning TypeNumber
Bad practice Warnings26
Correctness Warnings70
Internationalization Warnings13
Malicious code vulnerability Warnings5
Multithreaded correctness Warnings9
Performance Warnings77
Security Warnings45
Dodgy code Warnings138
Total383

SpotBugs increased!

@nextcloud-android-bot
Copy link
Collaborator

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

ezaquarii added a commit that referenced this pull request Apr 16, 2020
@ezaquarii
Start files sync job when enabling auto upload
34f724c 
This is #5885 solution ported to BackgroundJobManager API.

Fixes #5882

Signed-off-by: Chris Narkiewicz <hello@ezaquarii.com>
@ezaquarii ezaquarii mentioned this pull request Apr 16, 2020
Merged
@nextcloud-android-bot
Copy link
Collaborator

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

qrcode

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
Copy link
Collaborator

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

@nextcloud-android-bot
Copy link
Collaborator

Lint

Typestable-3.11PR
Warnings7777
Errors00

SpotBugs (new)

Warning TypeNumber
Bad practice Warnings26
Correctness Warnings70
Internationalization Warnings13
Malicious code vulnerability Warnings5
Multithreaded correctness Warnings9
Performance Warnings77
Security Warnings45
Dodgy code Warnings140
Total385

SpotBugs (stable-3.11)

Warning TypeNumber
Bad practice Warnings26
Correctness Warnings70
Internationalization Warnings13
Malicious code vulnerability Warnings5
Multithreaded correctness Warnings9
Performance Warnings77
Security Warnings45
Dodgy code Warnings139
Total384

SpotBugs increased!

@tobiasKaminsky
Do not run insertAllDBEntriesForSyncedFolder directly, but use FilesS…
c4a1c7a 
…yncJob

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
@tobiasKaminsky tobiasKaminsky merged commit f9fc4fb into stable-3.11 Apr 16, 2020
@delete-merged-branch delete-merged-branch bot deleted the uploadExisting branch April 16, 2020 21:41
@backportbot-nextcloud
Copy link

The backport to stable-3.11 failed. Please do this backport manually.

@ezaquarii ezaquarii mentioned this pull request Apr 18, 2020
Closed
ezaquarii added a commit that referenced this pull request Apr 18, 2020
@ezaquarii
Start files sync job when enabling auto upload
5ea217e 
This is #5885 solution ported to BackgroundJobManager API.

Fixes #5882
Fixes #5898

Signed-off-by: Chris Narkiewicz <hello@ezaquarii.com>
ezaquarii added a commit that referenced this pull request Apr 23, 2020
@ezaquarii
Start files sync job when enabling auto upload
d0a6fdc 
This is #5885 solution ported to BackgroundJobManager API.

Fixes #5882
Fixes #5898

Signed-off-by: Chris Narkiewicz <hello@ezaquarii.com>
@elementum elementum mentioned this pull request Dec 25, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ezaquarii ezaquarii ezaquarii approved these changes

@AndyScherzinger AndyScherzinger AndyScherzinger approved these changes

Assignees
No one assigned
Labels
3. to review
Projects
None yet
Milestone
Nextcloud App 3.11.1
Development

Successfully merging this pull request may close these issues.

Enabling auto-upload on Camera folder crashes app with ANR
4 participants
@tobiasKaminsky @nextcloud-android-bot @ezaquarii @AndyScherzinger