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

[TIMOB-26488] Android: Fix Ti.Filesystem.requestStoragePermissions for Android 8 and above #10442

Merged
merged 2 commits into from Nov 20, 2018

Conversation

ypbnv
Copy link
Contributor

@ypbnv ypbnv commented Nov 8, 2018

JIRA: https://jira.appcelerator.org/browse/TIMOB-26488

Description:
In Android 8 Google fixed a problem where granting a READ_EXTERNAL_STORAGE permissions automatically granted all the permissions in the group. This PR adds the WRITE_EXTERNAL_STORAGE permissions in the requestStoragePermissions() method in Filesystem module in order to not break existing apps relying on it providing write permission.

No unit test for this PR.

Test case:

This fix is only required for Android 8 and above.
Make sure you have both READ_EXTERNAL_STORAGE and WRITE_EXTERNAL_STORAGE declared in tiapp.xml
You can use any image placed in the Resources folder of the application.
After receiving a success callback you can check the devices Gallery\Photos application for the image. It may take some time (like 10 seconds) to appear there.
Before this PR you will always get the error callback.

app.js

let win = Ti.UI.createWindow({
        layout: 'vertical'
    }),
    permissionsButton = Ti.UI.createButton({
        title: 'Request permissions'
    }),
    addToGalleryReel = Ti.UI.createButton({
        title: 'Add to gallery reel',
        enabled: false,
        touchEnabled: false
    });

win.addEventListener('open', function () {
    addToGalleryReel.enabled = Ti.Filesystem.hasStoragePermissions();
    addToGalleryReel.touchEnabled = Ti.Filesystem.hasStoragePermissions();
});

permissionsButton.addEventListener('click', function () {
    if (!Ti.Filesystem.isExternalStoragePresent()) {
        alert('No external storage available.');
        return;
    }
    if (Ti.Filesystem.hasStoragePermissions()) {
        alert('Permissions already granted.')
    } else {
        Ti.Filesystem.requestStoragePermissions( function (requestCallback) {
            addToGalleryReel.enabled = requestCallback.success;
            addToGalleryReel.touchEnabled = requestCallback.success;
        });
    }
});

addToGalleryReel.addEventListener('click', function () {
    let file = Ti.Filesystem.getFile(Ti.Filesystem.resourcesDirectory, <your-image-here>);
    Ti.Media.saveToPhotoGallery(file, {
        success: function() { alert('Success.') },
        error: function() { alert('Error.') }
    });
});

win.add([permissionsButton, addToGalleryReel]);
win.open();

Copy link
Contributor

@garymathews garymathews left a comment

Choose a reason for hiding this comment

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

CR: PASS

@drauggres
Copy link
Contributor

https://developer.android.com/reference/android/Manifest.permission#READ_EXTERNAL_STORAGE

Any app that declares the WRITE_EXTERNAL_STORAGE permission is implicitly granted this permission.

So requesting only WRITE_EXTERNAL_STORAGE should be enough

@longton95 longton95 self-requested a review November 20, 2018 15:28
Copy link
Contributor

@longton95 longton95 left a comment

Choose a reason for hiding this comment

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

FR Passed

Tested using the tescase above, the permissions are requested succcessfully and the image is saved.

ENV

Nexus 6P (8.1.0)
Android Emulator (8.0, 8.1.0, 9.0)
macOS: 10.14.1
Windows 10 Pro: 1703
Appc NPM: 4.2.13
Appc CLI: 7.0.7
Ti CLI: 5.1.1
Node: 8.12.0
NPM: 6.4.1

Awaiting Jenkins

@build
Copy link
Contributor

build commented Nov 20, 2018

Messages
📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

@longton95 longton95 merged commit d758cd1 into tidev:master Nov 20, 2018
build pushed a commit to hansemannn/titanium_mobile that referenced this pull request Dec 3, 2018
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.

None yet

6 participants