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

#296: Add documents provider #1368

Merged
merged 2 commits into from
Jun 29, 2016
Merged

#296: Add documents provider #1368

merged 2 commits into from
Jun 29, 2016

Conversation

przybylski
Copy link
Member

@przybylski przybylski commented Dec 19, 2015

--------- edited by @jabarros
TASKs:

  • [QA] Create 'document_provider' branch in QA Repo @jesmrec
  • [QA] Merge 'document_provider' branch in QA Repo
  • [QA] Create test plan @jesmrec
  • [QA] Validate test plan @jesmrec

@tobiasKaminsky
Copy link
Contributor

Have you tested it?
Then I can do a code review and add it in beta?

@przybylski
Copy link
Member Author

works fine with multiple accounts, on nexus 4

@tobiasKaminsky
Copy link
Contributor

Maybe this would be a good use case to show it in the "what's new" slide show?

@tobiasKaminsky
Copy link
Contributor

Included in beta.
Please test @ALL

@przybylski
Copy link
Member Author

could be, but 'whats new' should be also included, this requires some addtional work from graphics people

@tobiasKaminsky
Copy link
Contributor

Yeah, then we should enforce this...
Who is responsible for the artwork?

@przybylski
Copy link
Member Author

No idea :D @AndyScherzinger said that he would take a look on images and texts

@tobiasKaminsky
Copy link
Contributor

@przybylski
#296 (comment) said:
The beta client seems to support opening via SAF, but not saving.

@przybylski
Copy link
Member Author

I know, for now I want to provide opening, saving will come in other PR

@tobiasKaminsky
Copy link
Contributor

@przybylski the current version shows only downloaded files, right?
It would be cool to have every file on the server and download them if necessary?
Or is it possible to show the download state to the user?

@przybylski
Copy link
Member Author

Last time I tested it, it was working just as you described it. Showing all the files and download then if necessary

@tobiasKaminsky
Copy link
Contributor

Maybe I have messed something up in beta.
Can you try it there?

@przybylski
Copy link
Member Author

The code looks fine on beta branch. Is it not downloading the file?
Currently my android device is far away from me so I cannot test it on the real data :/
The one confusing thing is that there will be no progress indicator on the activity but on the notification bar. But this is something that system is forcing so there si nothing we can do about it.

@tobiasKaminsky
Copy link
Contributor

The files / folders that are not already downloaded are not even shown, so I cannot click them.

@przybylski
Copy link
Member Author

Ok, I can see the problem,
when you were rebasing for beta branch you updated getFolderContent call to include true flag (only on device), it should be false on beta.

@tobiasKaminsky
Copy link
Contributor

Thanks :)
Now it is working! I will add it to beta soon.

Is it possible to show the download state?

@przybylski
Copy link
Member Author

No, DocumentProvider doesn't support progress indicator. The only download state is available in a notification bar, and it is normally invoked when ownCloud is downloading file..

@tobiasKaminsky
Copy link
Contributor

Ah, no, I meant the state of a file: downloaded on device / not downloaded.
As it might be interesting to see this in order to avoid triggering a big download when selecting a file.

@przybylski
Copy link
Member Author

There is no built in mechanism in DocumentsProvider to indicate this :/

@@ -112,17 +112,29 @@
android:name=".providers.FileContentProvider"
android:authorities="@string/authority"
android:enabled="true"
android:exported="false"
android:exported="true"
Copy link
Member

Choose a reason for hiding this comment

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

Is it necessary to make this existing Provider public?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, documents provider is queried by external application, and at that point its outside owncloud permission so exported content provider is required.

@przybylski przybylski closed this Jun 10, 2016
@davivel
Copy link
Contributor

davivel commented Jun 15, 2016

Hi, @przybylski .

Should we understand you don't want to contribute this code to ownCloud anymore?

Thanks in advance.

@przybylski
Copy link
Member Author

It's not that I don't want to, but the fact that such simple pr was opened for over half a year got me discouraged.

@davivel
Copy link
Contributor

davivel commented Jun 16, 2016

I can understand that. In any case, closing it doesn't seem a way to get it merged faster, does it?

Please, consider reopening it. Your contributions to this project always were more than important.

@przybylski
Copy link
Member Author

It didn't got it faster, BUT it got your attention so it kinda worked ;)

@przybylski przybylski reopened this Jun 16, 2016
@jabarros
Copy link
Contributor

@jesmrec this PR is prepared to be tested :)
@przybylski maybe a rebase on master is needed in order to have the branch updated. Could you do that?

@jesmrec
Copy link
Collaborator

jesmrec commented Jun 20, 2016

👍 @jabarros a test plan will be designed asap.

@jabarros
Copy link
Contributor

@jesmrec, rebase on master done

@jesmrec
Copy link
Collaborator

jesmrec commented Jun 27, 2016

Approved, nice work. Next step, ability to modify :) :)

@przybylski @jabarros A new rebase is needed, please.

@jabarros
Copy link
Contributor

Rebase done. Wating Travis.

@jabarros jabarros force-pushed the document_provider branch from ac072dd to 24619ce Compare June 29, 2016 06:36
@jabarros jabarros merged commit 181c20e into master Jun 29, 2016
@davivel davivel added this to the 2.1.0 milestone Jul 4, 2016
@davivel davivel deleted the document_provider branch August 9, 2016 14:51
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.

6 participants