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

Add documents provider #3

Merged
merged 3 commits into from
Jun 21, 2016
Merged

Add documents provider #3

merged 3 commits into from
Jun 21, 2016

Conversation

przybylski
Copy link
Member

No description provided.

android:name="org.nextcloud.providers.DocumentsStorageProvider"
android:exported="true"
android:grantUriPermissions="true"
android:permission="android.permission.MANAGE_DOCUMENTS"
Copy link
Member

Choose a reason for hiding this comment

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

Would this need a permission Api implementation for marshmallow and up?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so, the reason is that this provider is exported so user are specifically calling for document provider which shouldn't require any android requirements (it queries only app local resources).
But I cannot check that on a real device because of lack of android 6 device :(

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I finally need to acquire one ;)

Copy link
Member

Choose a reason for hiding this comment

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

I do have Android N Preview 3 on my phone so I can install the branch in the next days. Afk right now...

Copy link
Member

Choose a reason for hiding this comment

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

like I commented, tested on Android N Preview, works like a charm 👍

@przybylski przybylski changed the title #296: Add documents provider Add documents provider Jun 10, 2016
@przybylski
Copy link
Member Author

@tobiasKaminsky @AndyScherzinger review pls :)

final FileCursor result = new FileCursor(projection);

final OCFile browsedDir = mCurrentStorageManager.getFileById(folderId);
for (OCFile file : mCurrentStorageManager.getFolderContent(browsedDir))
Copy link
Member

Choose a reason for hiding this comment

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

please add { } to the content of the loop as this minified code is somehow confusing

@tobiasKaminsky
Copy link
Member

apart from minor code formatting it LGTM
Is this proper tested? I have never really used this one

@przybylski
Copy link
Member Author

I tested it on android pre6 and it was working fine on opening files.
As a continuation I wan't to allow writing as well but it is a little bit trickier.

@AndyScherzinger
Copy link
Member

I can't review before next week, so whoever has time to do so, feel free. I am on a workshop with work so I won't be able to. Maybe I find the time to review tomorrow before the workshop.
I am really sorry for that :(

@tobiasKaminsky
Copy link
Member

@przybylski there are a few comments on owncloud regarding this pr: owncloud/android#1368 (comment) have you checked them?

@AndyScherzinger
Copy link
Member

One issue that came up in the beta: https://help.nextcloud.com/t/owncloud-logo-in-beta-app/1013/3
We seem to still have an owncloud icon being used for display. I might be able to fix this this week in case you don't have the time @przybylski

@AndyScherzinger
Copy link
Member

Logo has been fixed: f054c7d

@AndyScherzinger
Copy link
Member

Will test drive the branch tomorrow.

@AndyScherzinger
Copy link
Member

Test has been successful on Nexus5X Android N Preview 4

@AndyScherzinger
Copy link
Member

👍

@AndyScherzinger
Copy link
Member

@przybylski the honor to push the merge button is yours 🎉

@AndyScherzinger AndyScherzinger added this to the Nextcloud App 1.1.0 milestone Jun 20, 2016
@przybylski przybylski merged commit 7a54dc6 into master Jun 21, 2016
@przybylski
Copy link
Member Author

🎉

@AndyScherzinger AndyScherzinger deleted the document_provider branch June 21, 2016 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants