FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Supply permission callback by skywalkerDavid · Pull Request #113 · airbnb/AirMapView · GitHub

/ AirMapView Public

Supply permission callback - #113

Open
skywalkerDavid wants to merge 3 commits into
masterfrom
dawei-location-permission
ConversationCommits (3)ChecksFiles changed
Open

Supply permission callback#113
skywalkerDavid wants to merge 3 commits into
masterfrom
dawei-location-permission

Conversation

skywalkerDavid commented Oct 31, 2017

Copy link
Copy Markdown

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 commented Oct 31, 2017

Copy link
Copy Markdown
Contributor

Is this the fix for #112?

felipecsl suggested changes Oct 31, 2017
View reviewed changes
Comment thread library/src/main/java/com/airbnb/android/airmapview/listeners/OnLocationPermissionListener.java
@@ -0,0 +1,7 @@
package com.airbnb.android.airmapview.listeners;

public interface OnLocationPermissionListener {

felipecsl Oct 31, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

can you please add javadocs to this interface?

Comment thread library/src/main/java/com/airbnb/android/airmapview/RuntimePermissionUtils.java Outdated
static void onRequestPermissionsResult(AirMapInterface airMapInterface, int requestCode,
int[] grantResults) {
public static void onRequestPermissionsResult(Activity activity, AirMapInterface airMapInterface, int requestCode,
String[] permissions, int[] grantResults) {

felipecsl Oct 31, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

revert formatting change?

Comment thread library/src/main/java/com/airbnb/android/airmapview/RuntimePermissionUtils.java Outdated
* Utility class that handles runtime permissions
*/
final class RuntimePermissionUtils {
public final class RuntimePermissionUtils {

felipecsl Oct 31, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

does this need to be public?

Comment thread library/src/main/java/com/airbnb/android/airmapview/RuntimePermissionUtils.java Outdated
}
}

return true;

felipecsl Oct 31, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

should use 2-space indentation instead

Comment thread library/src/main/java/com/airbnb/android/airmapview/RuntimePermissionUtils.java Outdated
return false;
}

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

felipecsl Oct 31, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

you can use Collections.singleton() instead

skywalkerDavid commented Nov 1, 2017

Copy link
Copy Markdown
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 commented Nov 1, 2017

Copy link
Copy Markdown
Author

Comments addressed.

skywalkerDavid commented Nov 6, 2017

Copy link
Copy Markdown
Author

@felipecsl PTAL, thank you

felipecsl suggested changes Nov 6, 2017
View reviewed changes

felipecsl left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks almost ready, just a couple of nits

Comment thread library/src/main/java/com/airbnb/android/airmapview/RuntimePermissionUtils.java
if (permissions.length != LOCATION_PERMISSIONS.length) {
return false;
}

felipecsl Nov 6, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

Comment thread library/src/main/java/com/airbnb/android/airmapview/RuntimePermissionUtils.java
} else if (!shouldShowRequestPermissionRationale(activity, LOCATION_PERMISSIONS)) {
airMapInterface.onLocationPermissionsNeverAskAgain();
} else {
airMapInterface.onLocationPermissionsDenied();

felipecsl Nov 6, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

indentation is still 4 spaces here

Comment thread library/src/main/java/com/airbnb/android/airmapview/WebViewMapFragment.java

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

felipecsl Nov 6, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

formatting

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
felipecsl felipecsl requested changes
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL