Improve DocumentsProvider and add tests#6806
Merged
Merged
Conversation
grote
force-pushed
the
grote/improve-documentsprovider-add-tests
branch
2 times, most recently
from
August 26, 2020 15:36
e9a53b7 to
940affd
Compare
grote
marked this pull request as ready for review
August 26, 2020 16:57
12 tasks
The initial goal was to make all operations completely synchronous, so failures can be detected by apps using our DocumentsProvider. However, `#openDocument()` could not be made fully synchronous, because we need to wait for the ParcelFileDescriptor we are returning to close before we can upload a file. Nextcloud works with locally cached files that only get synchronized to remote storage. Things I have tried: * Use the Handler from the calling I/O Thread: not guaranteed to have a prepared Looper, so can't always create Handler on that Thread * Extend ParcelFileDescriptor and override its close() methods: For some reason they don't get called when the stream gets closed * notify the content URI when the upload is complete, so callers can wait for the notification: works, but is non-standard. Other DPs are not doing it, so it requires Nextcloud specific code on caller side and is still hacky: what happens if notification doesn't come? Timeout, but for how long? * use other ways to get a ParcelFileDescriptor on the file: couldn't find anything that would interface with Nextcloud's current architecture Signed-off-by: Torsten Grote <t@grobox.de>
Note there's still some FIXMEs in the tests that hint at bugs discovered by the tests. Also this is very incomplete, but a good beginning for further tests. Signed-off-by: Torsten Grote <t@grobox.de>
grote
force-pushed
the
grote/improve-documentsprovider-add-tests
branch
from
August 27, 2020 14:46
940affd to
27f48b3
Compare
… [skip ci] Signed-off-by: nextcloud-android-bot <android@nextcloud.com>
Member
Author
|
@jmue maybe you also want to have a look! |
|
|
||
| object DocumentsProviderUtils { | ||
|
|
||
| internal fun DocumentFile.getOCFile(storageManager: FileDataStorageManager): OCFile? { |
Member
There was a problem hiding this comment.
Is there a reason to do this?
Other than that it is now in Kotlin? :)
tobiasKaminsky
approved these changes
Sep 2, 2020
Collaborator
|
APK file: https://www.kaminsky.me/nc-dev/android-artifacts/15677.apk |
Collaborator
|
master-light-blue-Screenshot test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/15677-Screenshot-master-light-blue |
Collaborator
|
stable-light-blue-Screenshot test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/15677-Screenshot-stable-light-blue |
Collaborator
Codacy401Lint
SpotBugs (new)
SpotBugs (master)
|
delete-merged-branch
Bot
deleted the
grote/improve-documentsprovider-add-tests
branch
September 2, 2020 10:27
Contributor
Member
Author
|
Thanks @jmue for testing and filing those tickets! The first one I noticed as well, but decided not to tackle it within this PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR improves the implementation of the
DocumentsProviderand adds first instrumentation tests.Testing
Closes #6753