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

Fix compile error when request SYSTEM_ALERT_WINDOW on SupportFragment #482

Merged
merged 1 commit into from
Jun 5, 2018

Conversation

tomoya0x00
Copy link
Contributor

This problem seems to have happened because android.support.v4.app.Fragment.getActivity() is specified as nullable.

And I also fix #429.

@shiraji
Copy link
Member

shiraji commented Jun 4, 2018

Current generated code where we got syntax error for activity.getPackageName() and activity.startActivityForResult(intent, REQUEST_FOO)

fun ContactsFragment.fooWithPermissionCheck() {
  if (PermissionUtils.hasSelfPermissions(activity, *PERMISSION_FOO) || Settings.canDrawOverlays(activity)) {
    foo()
  } else {
    val intent = Intent(Settings.ACTION_MANAGE_OVERLAY_PERMISSION, Uri.parse("package:" + activity.getPackageName()))
    activity.startActivityForResult(intent, REQUEST_FOO)
  }
}

fun ContactsFragment.onActivityResult(requestCode: Int) {
  when (requestCode) {
    REQUEST_FOO -> {
      if (PermissionUtils.hasSelfPermissions(activity, *PERMISSION_FOO) || Settings.canDrawOverlays(activity)) {
        foo()
      }
    }
  }
}

Generated Code

fun ContactsFragment.fooWithPermissionCheck() {
  if (PermissionUtils.hasSelfPermissions(this.activity, *PERMISSION_FOO) || Settings.canDrawOverlays(this.activity)) {
    foo()
  } else {
    val intent = Intent(Settings.ACTION_MANAGE_OVERLAY_PERMISSION, Uri.parse("package:" + this.activity!!.getPackageName()))
    this.startActivityForResult(intent, REQUEST_FOO)
  }
}

fun ContactsFragment.onActivityResult(requestCode: Int) {
  when (requestCode) {
    REQUEST_FOO -> {
      if (PermissionUtils.hasSelfPermissions(this.activity, *PERMISSION_FOO) || Settings.canDrawOverlays(this.activity)) {
        foo()
      }
    }
  }
}

@@ -14,7 +14,7 @@ class KotlinSupportFragmentProcessorUnit(messager: Messager) : KotlinBaseProcess

override fun getTargetType(): TypeMirror = typeMirrorOf("android.support.v4.app.Fragment")

override fun getActivityName(): String = "activity"
override fun getActivityName(targetParam: String): String = "$targetParam.activity"
Copy link
Member

Choose a reason for hiding this comment

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

I wonder can we ignore targetParam here?

override fun getActivityName(targetParam: String): String = "activity"

Copy link
Member

Choose a reason for hiding this comment

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

The answer is No! We have PermissionRequest class

val intent = Intent(Settings.ACTION_MANAGE_OVERLAY_PERMISSION, Uri.parse("package:" + target.activity!!.getPackageName()))

@shiraji
Copy link
Member

shiraji commented Jun 4, 2018

We really need compiling test for kotlin now...

Copy link
Member

@shiraji shiraji left a comment

Choose a reason for hiding this comment

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

We may need to discuss about !! but at this moment, we can merge this in.

Copy link
Member

@hotchemi hotchemi left a comment

Choose a reason for hiding this comment

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

Thank you so much! is it possible to avoid using !!? 👁

builder.addStatement("val intent = %T(%T.ACTION_MANAGE_OVERLAY_PERMISSION, %T.parse(\"package:\" + %N.getPackageName()))", INTENT, SETTINGS, URI, activity)
builder.addStatement("%N.startActivityForResult(intent, %N)", activity, requestCodeField)
override fun addRequestPermissionsStatement(builder: FunSpec.Builder, targetParam: String, activityVar: String, requestCodeField: String) {
builder.addStatement("val intent = %T(%T.ACTION_MANAGE_OVERLAY_PERMISSION, %T.parse(\"package:\" + %N!!.getPackageName()))", INTENT, SETTINGS, URI, activityVar)
Copy link
Member

@shiraji shiraji Jun 5, 2018

Choose a reason for hiding this comment

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

We have 2 options to not to use !!

  1. We can replace !! with ?. Like parse("package: " + activity?.getPackageName()) it ends up parse("package: null").

  2. However, if we could add null check to Java version, we can do early return in kotlin so that we don't need to worry about this problem. (This generated code still have problem with startActivityForResult)

fun ContactsFragment.fooWithPermissionCheck() {
  val activity = activity ?: return // add this 
  if (PermissionUtils.hasSelfPermissions(activity, *PERMISSION_FOO) || Settings.canDrawOverlays(activity)) {
    foo()
  } else {
    val intent = Intent(Settings.ACTION_MANAGE_OVERLAY_PERMISSION, Uri.parse("package:" + activity.getPackageName()))
    activity.startActivityForResult(intent, REQUEST_FOO)
  }
}

What do you think ? @hotchemi @tomoya0x00

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we should do this in this PR. We can ship this PR and then later version we can add null check in both Java and Kotlin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I missed this comment.

I think we can add a null check later.

I think that early return is better. So We already use it in PermissionRequest class, I feel a sense of unity.

Copy link
Member

Choose a reason for hiding this comment

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

Yea I definitely prefer the latter one. But it can be another PR for sure~

@tomoya0x00
Copy link
Contributor Author

@shiraji

We really need compiling test for kotlin now...

I think so too...

Does compiling test for Java exist?
I would like to refer if it exists.

@hotchemi

is it possible to avoid using !!?

I'll try it.
If activity is null, what do you think of the idea of ​​throw IllegalStateException ?

@shiraji
Copy link
Member

shiraji commented Jun 5, 2018

@tomoya0x00

Does compiling test for Java exist?

compile-testing

There is no kotlin compiler testing that tests kapt generated test. @mannodermaus asks moshi team to open kotlin code gen test class

https://github.com/square/moshi/blob/3ecdfb6374758678859d1a75cb1b890ac28656fa/kotlin/codegen/src/test/java/com/squareup/moshi/kotlin/codegen/JsonClassCodegenProcessorTest.kt

@hotchemi
Copy link
Member

hotchemi commented Jun 5, 2018

OK let me merge! Hopefully someone would address early return..? cc: @tomoya0x00 😁

@hotchemi hotchemi merged commit 15143f1 into permissions-dispatcher:master Jun 5, 2018
@tomoya0x00 tomoya0x00 deleted the fix_issue_429 branch June 5, 2018 14:18
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.

Fragment's onActivityResult() is not called when requesting SYSTEM_ALERT_WINDOW permission
3 participants