Refactor to delegate to helpers #110
Conversation
There was a problem hiding this comment.
This is a pretty sweet refactor, it makes the EasyPermissions class way nicer! 😁
The one issue I see that would make me hesitate to update to v0.4 is that we're back to passing in Objects instead of the type checked Activitys and Fragments. I've elaborated further below.
| */ | ||
| public static void requestPermissions(@NonNull Activity activity, | ||
| public static void requestPermissions(@NonNull Object host, |
There was a problem hiding this comment.
@samtstern I feel like this is a step backwards in terms of the library's usability for the end user. I really liked the fact that v0.3 of EP had overloads for the activities and fragments. I do understand that you're trying to simplify the main EP class, but I feel like the priority is making a nice, type checked api for the end user.
In addition, you have PermissionHelper.newInstance(object) setup to accept any object without fuss so adding the other method overloads wouldn't require much extra work. (Excluding documentation copypasta)
| private T mHost; | ||
|
|
||
| @NonNull | ||
| public static PermissionHelper getInstance(Object host) { |
There was a problem hiding this comment.
This method name confused before I looked at its implementation: I think it should be called newInstance. Otherwise, it looks like you're getting a single PermissionHelper instance per Activity/Fragment (which set off alarm bells in my mind about memory leaks 🙀)
| // ============================================================================ | ||
|
|
||
| public PermissionHelper(@NonNull T host) { | ||
| this.mHost = host; |
There was a problem hiding this comment.
Style nit: this is why I like hungarian notation, no need to use this. 😁
| @RequiresApi(api = Build.VERSION_CODES.M) | ||
| public static boolean somePermissionDenied(@NonNull android.app.Fragment fragment, @NonNull String[] perms) { | ||
| return shouldShowRationale(fragment, perms); | ||
| public static boolean somePermissionDenied(@NonNull Object host, @NonNull String[] perms) { |
There was a problem hiding this comment.
While we're refactoring, I just noticed: why aren't we using vargs? It would be a small yet sweet improvement.
|
@SUPERCILEX thanks for taking a look. You're right, keeping types is worth it (I was being lazy). Pretty sure I now have things to a point where we have no casting to |
|
|
|
@samtstern Awesome!!! 😁 PS: how did you generate that compatibility report? It looks really cool! |
|
@SUPERCILEX I found this really cool tool the other day: Here's my lame script for using it # Variables
VERSION_1="0.3.1"
VERSION_2="0.4.0"
# Copy the two AARs into your local directory
cp ~/.m2/repository/pub/devrel/easypermissions/$VERSION_1/easypermissions-$VERSION_1.aar .
cp ~/.m2/repository/pub/devrel/easypermissions/$VERSION_2/easypermissions-$VERSION_2.aar .
# Unzip them into temp directories
mkdir $VERSION_1 $VERSION_2
unzip -d $VERSION_1 easypermissions-$VERSION_1.aar
unzip -d $VERSION_2 easypermissions-$VERSION_2.aar
# Compare them
japi-compliance-checker --lib=easypermissions \
$VERSION_1/classes.jar $VERSION_2/classes.jar |
|
@samtstern Ooooooooo, that's really cool! I'll have to check it out with FirebaseUI |
Makes the EasyPermissions class less of a monster and centralizes switching on API version or host object class.