Skip to content

Conversation

@comigor
Copy link
Contributor

@comigor comigor commented Feb 19, 2018

Some third-party native components should have custom Android permissions (eg. android.permission.CAMERA) on the container's AndroidManifest.xml, so it should be configurable.

Also, it's good practice to implement react-native's PermissionAwareActivity so on newer versions of Android, the user will be prompted to accept it on runtime.

@belemaire belemaire requested a review from deepueg February 20, 2018 17:53
return PackageManager.PERMISSION_GRANTED;
}

@Override
Copy link
Contributor

Choose a reason for hiding this comment

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

please add @RequiresApi(api = Build.VERSION_CODES.M) to avoid lint warning


@Override
public int checkPermission(String permission, int pid, int uid) {
return PackageManager.PERMISSION_GRANTED;
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is supposed to return PERMISSION_GRANTED or PERMISSION_DENIED based on if the permission is allowed for a particular pid/uid. Overriding with permission granted may have an inverse effect for the feature ?.

@Override
public void requestPermissions(String[] permissions, int requestCode, PermissionListener listener) {
mPermissionListener = listener;
ActivityCompat.requestPermissions(this, permissions, requestCode);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please call mReactActivityDelegate. requestPermissions() as the delegate now extends the ReactActivityDelegate.


@Override
public void onRequestPermissionsResult(int requestCode, @NonNull String[] permissions, @NonNull int[] grantResults) {
mPermissionListener.onRequestPermissionsResult(requestCode, permissions, grantResults);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please call mReactActivityDelegate. onRequestPermissionsResult() as the delegate now extends the ReactActivityDelegate.

}

@Override
public int checkPermission(String permission, int pid, int uid) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be okay to remove this method as it's returning default int value (there is no logic involved)

}

@Override
public int checkSelfPermission(String permission) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be okay to remove this method as it's returning default int value (there is no logic involved)

@krunalsshah
Copy link
Contributor

@Igor1201 thanks for turning the PR, we were planning to implement the PermissionAwareActivity for this release , Glad you picked it up 👍

@comigor
Copy link
Contributor Author

comigor commented Feb 21, 2018

@krunalsshah Nice to know we're in sync! :)
@deepueg I've made some changes to the code. Could you review it again?

@deepueg
Copy link
Contributor

deepueg commented Feb 21, 2018

Looks good

@deepueg deepueg merged commit f7d05ba into electrode-io:master Feb 21, 2018
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