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

Android: Check filesystem permission if filesystem sync target is selected #1665

Merged
merged 3 commits into from Jul 13, 2019

Conversation

@xuhcc
Copy link
Contributor

commented Jun 17, 2019

Tested on LineageOS 15.1 (Android 8.1) and Android 9.
Related: #1654, #1658

@xuhcc xuhcc force-pushed the xuhcc:android-filesystem-permission branch from 8e90d8e to a653db0 Jun 17, 2019

@tessus tessus added the android label Jun 17, 2019

@laurent22

This comment has been minimized.

Copy link
Owner

commented Jun 19, 2019

Last time I tried, PermissionAndroid wasn't working properly. I had this bug: https://stackoverflow.com/questions/49771084/permission-always-returns-never-ask-again

Is it working for you? And what will happen if, like on the post above, permission always returns "never_ask_again"?

) {
Alert.alert(
_('Permission error'),
_(

This comment has been minimized.

Copy link
@laurent22

laurent22 Jun 19, 2019

Owner

Please put this string on one line, as otherwise I don't think it will be picked up by the translation software.

This comment has been minimized.

Copy link
@xuhcc

xuhcc Jun 20, 2019

Author Contributor

Do you use xgettext for extracting strings? I just tried and it gave me correct result:

$ xgettext --version
xgettext (GNU gettext-tools) 0.19.8.1
$ xgettext lib/components/screens/config.js --output=test.po --language=JavaScript --no-location
#: lib/components/screens/config.js:43
msgid ""
"Filesystem permission denied. Please choose another synchronization target."
msgstr ""
{
title: _('Permission to write to external storage'),
message: _(
'In order to use file system synchronization your ' +

This comment has been minimized.

Copy link
@laurent22

laurent22 Jun 19, 2019

Owner

Same here, it should be on one line.

&& !await this.checkFilesystemPermission()
) {
Alert.alert(
_('Permission error'),

This comment has been minimized.

Copy link
@laurent22

laurent22 Jun 19, 2019

Owner

Please name it something like "Sync target error" otherwise it looks like it's the settings that cannot be written to disk due to a permission error.

@laurent22

This comment has been minimized.

Copy link
Owner

commented Jun 19, 2019

For the save error message, let's do it like this:

  • Title: _('Warning')
  • Message: _('Joplin does not have permission to access "%s". Either choose a different sync target, or give Joplin the "Storage" permission.', this.state.settings['sync.2.path'])

When displaying this error, save the settings anyway in case the user has changed other settings.

@xuhcc

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

Last time I tried, PermissionAndroid wasn't working properly. I had this bug: https://stackoverflow.com/questions/49771084/permission-always-returns-never-ask-again

Is it working for you? And what will happen if, like on the post above, permission always returns "never_ask_again"?

Yes, it's working. There a few reports about this bug but in most cases it is not confirmed by the RN team. Here someone says that bug was fixed in 0.59.9.

If PermissionsAndroid.request returns 'never_ask_again', the app will prevent user from saving settings. We can add option to ignore permission error in this case.

@xuhcc

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

Side note: APK build process started to fail, and changes from commit 035b9c6 didn't solve the problem for me. I got it working by adding these lines to ReactNativeClient/lib/android/build.gradle:

    googlePlayServicesVersion = "16.0.0"
    googlePlayServicesVisionVersion = "17.0.2"

As recommended here facebook/react-native#25292 (comment)

@laurent22

This comment has been minimized.

Copy link
Owner

commented Jul 13, 2019

Looks good, thanks @xuhcc!

@laurent22 laurent22 merged commit f6b0da3 into laurent22:master Jul 13, 2019

@xuhcc xuhcc referenced this pull request Aug 1, 2019
2 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.