Skip to content

Conversation

jackrzhang
Copy link
Contributor

The first time a user attempts to download an image, permission to write to external storage (the SD card) should be requested. If permission is granted by the user, the image should proceed to download.

If permission is denied, the image will not be fetched, and the corresponding text 'Storage permission denied' is shown. Any subsequent time that a user attempts to download an image, a 'rationale' modal will appear with explaining why the app needs permissions, and the permission will once again be requested.

android-request-permissions-write-external-storage-1

If a user instructs that the permission not be asked for again, then subsequent attempts to download an image will only result in showing the text 'Storage permission denied', though the permission can always be changed via Android Settings.

android-request-permissions-write-external-storage-2

For reference, see React Native 0.57's PermissionsAndroid API and Android's developer documentation on requesting permissions.

This fixes #3115 and progresses #3075.
@gnprice

@gnprice gnprice added the review label Nov 9, 2018

const actionSheetButtons: ButtonType[] = [
{ title: 'Download file', onPress: downloadImage },
{ title: 'Download image', onPress: tryToDownloadImage },
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure to update the messages_en.json file.

Copy link
Contributor Author

@jackrzhang jackrzhang Nov 9, 2018

Choose a reason for hiding this comment

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

Will update! I wonder if there's any way that this could be caught via the test suite or some other automated check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, not without a complicated (but exciting!) side project.... likely involving Babel and analyzing strings used vs translated.

@jackrzhang jackrzhang force-pushed the android-external-write branch from e4b1f11 to 2e8cf18 Compare November 9, 2018 02:11
@jackrzhang
Copy link
Contributor Author

(Also, this moves forward #2618.)

jackrzhang and others added 6 commits November 9, 2018 18:38
Resolve a naming collision in LightboxActionSheet.js by renaming
the function previously named `downloadFile` to `tryToDownloadImage`.
Fixes zulip#3115.  See React Native 0.57's PermissionsAndroid API.
This helps make the logic a bit easier to follow.
I think these can still be improved (they still feel a little stuffy),
but this should be good enough.
@gnprice gnprice force-pushed the android-external-write branch from 2e8cf18 to 07354a3 Compare November 10, 2018 03:03
@gnprice gnprice merged commit 07354a3 into zulip:master Nov 10, 2018
@gnprice gnprice removed the review label Nov 10, 2018
@gnprice
Copy link
Member

gnprice commented Nov 10, 2018

Thanks @jackrzhang ! Merged.

I made a few edits on top that I think make the code easier to understand -- please take a look.

I missed @borisyankov 's comment above (sorry!) until I'd already merged:

Make sure to update the messages_en.json file.

But actually I think the situation is that these messages -- all the labels in the lightbox action sheet -- are already untranslated even when our volunteer translators have kindly provided translations. 😞 Note the lack of any code to act on these strings to find their translations. I also quickly verified just now with German.

So that would be good to fix. Also while we're at it make the new messages in this new flow get translated too. :-)

@gnprice
Copy link
Member

gnprice commented Nov 10, 2018

(#2812 is some background for that translation issue.)

@jackrzhang jackrzhang deleted the android-external-write branch November 12, 2018 19:40
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.

android: Fix use of WRITE_EXTERNAL_STORAGE permission.

3 participants