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

Supply permission callback #113

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

skywalkerDavid
Copy link

Descriptions:

Supply the permission callbacks.

  1. Supply the denied and neverAskAgain callback
  2. Supply onCheckLocationPermissionResult, since onRequestPermissionsResult in NativeGoogleMapFragment and WebviewMapFragment won't be called when requestPermissions.

Test:

Tested with Sample app

Reviewers:

@felipecsl @gpeal @nwadams

@felipecsl
Copy link
Contributor

Is this the fix for #112?

@@ -0,0 +1,7 @@
package com.airbnb.android.airmapview.listeners;

public interface OnLocationPermissionListener {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you please add javadocs to this interface?

static void onRequestPermissionsResult(AirMapInterface airMapInterface, int requestCode,
int[] grantResults) {
public static void onRequestPermissionsResult(Activity activity, AirMapInterface airMapInterface, int requestCode,
String[] permissions, int[] grantResults) {
Copy link
Contributor

Choose a reason for hiding this comment

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

revert formatting change?

import static android.support.v4.content.PermissionChecker.checkSelfPermission;

/**
* Utility class that handles runtime permissions
*/
final class RuntimePermissionUtils {
public final class RuntimePermissionUtils {
Copy link
Contributor

Choose a reason for hiding this comment

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

does this need to be public?

}
}

return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

should use 2-space indentation instead

return false;
}

final Set<String> requestPermissions = new HashSet<>(Arrays.asList(LOCATION_PERMISSIONS));
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use Collections.singleton() instead

@skywalkerDavid
Copy link
Author

@felipecsl yes, it fixes the #112 and 1 other issue about permission: setMyLocationEnabled won't work immediately even after the permission is accepted.

Thank you for the comments, they are all very good, I will address them soon.

@skywalkerDavid
Copy link
Author

Comments addressed.

@skywalkerDavid
Copy link
Author

@felipecsl PTAL, thank you

Copy link
Contributor

@felipecsl felipecsl left a comment

Choose a reason for hiding this comment

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

Looks almost ready, just a couple of nits

return false;
}

final Set<String> requestPermissions = new HashSet<>(Arrays.asList(LOCATION_PERMISSIONS));
Copy link
Contributor

Choose a reason for hiding this comment

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

it's not clear to me why you're creating this set with a single item inside?
can you just check if permission.equals(LOCATION_PERMISSIONS) on line 55?

} else if (!shouldShowRequestPermissionRationale(activity, LOCATION_PERMISSIONS)) {
airMapInterface.onLocationPermissionsNeverAskAgain();
} else {
airMapInterface.onLocationPermissionsDenied();
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation is still 4 spaces here

}

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

Choose a reason for hiding this comment

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

formatting

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.

2 participants