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

[FEATURE REQUEST] Make document provider visibility independent from locking methods #3538

Merged
merged 14 commits into from
Feb 8, 2022

Conversation

JuancaG05
Copy link
Collaborator

@JuancaG05 JuancaG05 commented Feb 1, 2022

@JuancaG05 JuancaG05 self-assigned this Feb 1, 2022
@JuancaG05 JuancaG05 marked this pull request as ready for review February 1, 2022 13:12
@JuancaG05 JuancaG05 force-pushed the feature/doc_provider_visibility branch from 6b7e49c to 08b6cf9 Compare February 1, 2022 13:30
Copy link
Contributor

@abelgardep abelgardep left a comment

Choose a reason for hiding this comment

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

Just a question related to UX, but the code looks good! Good job @JuancaG05

Comment on lines 197 to 198
prefLockAccessDocumentProvider?.setOnPreferenceChangeListener { _: Preference?, newValue: Any ->
if (!(newValue as Boolean)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if this is the expected behavior 🤔

Not sure if we should show a dialog to the user when he wants to enable the documents provider(which is the expected behavior) from a UX point of view

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMHO, the dialog does not make sense here since access to document provider is open by default. Warning users when the Lock option is switched off would be the same as warning when the Security section is opened. I'd remove the dialog because the feature meaning is opposite as the previous one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I will remove it then. Thank you!

@jesmrec
Copy link
Collaborator

jesmrec commented Feb 2, 2022

(1) [DONE]

this one: #3538 (comment)

@jesmrec
Copy link
Collaborator

jesmrec commented Feb 2, 2022

(2)

I noticed a tiny difference with the old behaviour

  1. Install app
  2. Go to Settings without adding an account)
  3. Enable Lock access from document provider
  4. Open any app that implements document provider

Current: owncloud is shown as Locked

Expected (before): owncloud is hidden because no accounts were attached yet

Good to fix, but not a blocker

Pixel2 V11
08b6cf94

@jesmrec
Copy link
Collaborator

jesmrec commented Feb 7, 2022

(3) [FIXED]

Please, don't forget removing the following entry from the Changelog:

Enhancement - Allow access from document provider preference: [#3379](https://github.com/owncloud/android/issues/3379)

it does not make sense anymore, since the current one covers totally the final result.

@fesave fesave force-pushed the feature/doc_provider_visibility branch 3 times, most recently from 3b92617 to 6268654 Compare February 8, 2022 08:12
@fesave
Copy link
Contributor

fesave commented Feb 8, 2022

(3)

Please, don't forget removing the following entry from the Changelog:

Enhancement - Allow access from document provider preference: [#3379](https://github.com/owncloud/android/issues/3379)

it does not make sense anymore, since the current one covers totally the final result.

Removed 😉

@fesave fesave force-pushed the feature/doc_provider_visibility branch from e05a78f to 03c7876 Compare February 8, 2022 08:32
@fesave
Copy link
Contributor

fesave commented Feb 8, 2022

(3)

Please, don't forget removing the following entry from the Changelog:

Enhancement - Allow access from document provider preference: [#3379](https://github.com/owncloud/android/issues/3379)

it does not make sense anymore, since the current one covers totally the final result.

I think it's fixed, if you can test it thoroughly to make sure I'd appreciate it, anything you see, let me know.

@jesmrec
Copy link
Collaborator

jesmrec commented Feb 8, 2022

Everything is correct!! approved on my side and right to 2.20

@abelgardep abelgardep force-pushed the feature/doc_provider_visibility branch from f8fa7cb to 2c1f93b Compare February 8, 2022 16:05
@abelgardep abelgardep force-pushed the feature/doc_provider_visibility branch from 6571f72 to f72ad92 Compare February 8, 2022 16:18
@abelgardep abelgardep merged commit d6f4cb4 into master Feb 8, 2022
@abelgardep abelgardep deleted the feature/doc_provider_visibility branch February 8, 2022 17:09
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.

[FEATURE REQUEST] Make document provider visibility independent from locking methods
4 participants