Skip to content

M permissions removed: not mandatory#227

Merged
Cool04ek merged 3 commits intoYalantis:masterfrom
dejandobnikar:feature/removeStoragePermission
Nov 15, 2016
Merged

M permissions removed: not mandatory#227
Cool04ek merged 3 commits intoYalantis:masterfrom
dejandobnikar:feature/removeStoragePermission

Conversation

@dejandobnikar
Copy link
Copy Markdown
Contributor

@dejandobnikar dejandobnikar commented Nov 10, 2016

#212

If image is picked from the gallery, temporary permission to access an image is granted. Cropping destination can be set to the path returned from Context.getExternalFilesDir(), which, starting with KITKAT, does not need external storage permission.

Permission to save cropped image to Downloads folder is still required however.

This replicates 1.5.0 behaviour where External storage permission was not needed. It also prevents lib from crashing if permission not granted.

dejandobnikar and others added 2 commits November 10, 2016 22:15
maxSdkVersion removed - needed for saving image to Downloads
@Cool04ek
Copy link
Copy Markdown
Contributor

Changes in BitmapLoadTask is exactly what i was talking about!
But I don't see any point in changing Sample app as long as it's just a Sample of usage. And going along with your solution constricts options of using app. So, please, provide PR only with changes to BitmapLoadTask so I can accept it.

@dejandobnikar dejandobnikar force-pushed the feature/removeStoragePermission branch from a244a6f to bd3a8bb Compare November 15, 2016 10:08
@dejandobnikar
Copy link
Copy Markdown
Contributor Author

Sample app is restored, leaving changes to the BitmapLoadTask only.

}
} else if ("content".equals(inputUriScheme)) {
String path = FileUtils.getPath(mContext, mInputUri);
String path = getFilePath();
Copy link
Copy Markdown
Contributor

@Cool04ek Cool04ek Nov 15, 2016

Choose a reason for hiding this comment

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

I think it's better to check for permission in here. And in case there is no one, then throw exception.
Something like throw new IllegalStateException("No permission for storage was provided"); Bacause now if just null returned, than user of the lib will get NoSuchFileEcxeption and it's misleading.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The whole point of this PR is that Storage permission is not mandatory to read image file.

Fore example, if user selects image from an external app (Gallery etc), then temporary permission is granted for your app to read image file from specific Uri.
In other words, app has permission to read image file from Uri, but not to query ContentProvider.

With null returned we avoid querying ContentProvider, instead we try to access the file directly (via copyFile).

This way we are able to pick an image from gallery without Storage permission.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yep, that makes sense. Thank you very much.

@DejanRistic
Copy link
Copy Markdown

@Cool04ek
Is there a version of the lib on Maven that has this change in it? I think the latest out there is still 2.2.0 and this issue still persists.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants