Add PendingIntentCompat to enforce mutability flag

[adaext]

PendingIntentCompat combines mutability flag when necessary.

[adaext]

There are some errors in this PR, I am fixing it. Please don't merge it immediately, thanks.

[barbeau]

@adaext FYI, you could change this PR to a "draft" while you're still working on it:
https://github.blog/2019-02-14-introducing-draft-pull-requests/

This way it's clear to everyone that it's not ready for merge yet.

[adaext]

@adaext FYI, you could change this PR to a "draft" while you're still working on it: https://github.blog/2019-02-14-introducing-draft-pull-requests/

This way it's clear to everyone that it's not ready for merge yet.

Thank you very much, you are so kind. I've converted it to draft.

[barbeau]

@adaext Thanks for this PR!

Before diving into the details, I had a question about the broader reason for this pull request. It looks like it replaces around 6 lines of code with 393, and the majority of that new code is unused. Is there a reason for that new code to exist? Is there an immediate plan to use it in the project? From a maintenance perspective I'd prefer to keep smaller simpler solutions if that's all that's needed.

[adaext]

@adaext Thanks for this PR!

Before diving into the details, I had a question about the broader reason for this pull request. It looks like it replaces around 6 lines of code with 393, and the majority of that new code is unused. Is there a reason for that new code to exist? Is there an immediate plan to use it in the project? From a maintenance perspective I'd prefer to keep smaller simpler solutions if that's all that's needed.

Thanks for the question. Firstly, Everytime we use PendingIntent, we have to make operating system version judgement and decide whether to add or not add Mutability Flags. Since PendingIntent is often used, I think it's necessary to encapsulate these inside a new class. That's why I've written PendingIntentCompat. Secondly, I am trying to merge this new class into Android X, even though it succeed, it still need some time before it could be applied in this project. After this was accepted and could be used here, I will delete PendingIntentCompat and replace the code.

[barbeau]

Secondly, I am trying to merge this new class into Android X,

@adaext I appreciate your effort on this PR, but I think Android X would be a better home for this class.

IMHO, looking closer, there is just too much extra code here that's not currently needed to justify merging into this app - as a result, it adds a maintenance burden without any benefit. Only one of the PendingIntentCompat methods is used currently in a single location in the code (which actually increases the number of lines of code needed to implement the solution), and I don't see an immediate use for the other methods. There is also code to test back to API levels 22 and 23 (and tests are good!), but the minSdkVersion for this app is 24 (N), so those cases will never be seen. These tests make a lot more sense in something like Android X that has more API levels to support.

Please keep us posted on when this code gets released into Android X, and at that time we can switch over to using it if the benefits outweigh the costs.

For better coordination, I'd suggest opening issues in the future to discuss code changes before implementing them. I know issues aren't currently enabled on this GitHub repo, so I'm going to check with the team to see what the best path for coordination is going forward.

Thanks again for your contributions!

[mohammedkhider]

I agree with @barbeau that we should not add code that is not used to the App. This will increase the code size and bring maintenance burden. You can keep the methods that are used and needed.
Regarding adding this to Android-X, this is a very good idea. Let me know if you want me to connect you to the right people.

[adaext]

Secondly, I am trying to merge this new class into Android X,

@adaext I appreciate your effort on this PR, but I think Android X would be a better home for this class.

IMHO, looking closer, there is just too much extra code here that's not currently needed to justify merging into this app - as a result, it adds a maintenance burden without any benefit. Only one of the PendingIntentCompat methods is used currently in a single location in the code (which actually increases the number of lines of code needed to implement the solution), and I don't see an immediate use for the other methods. There is also code to test back to API levels 22 and 23 (and tests are good!), but the minSdkVersion for this app is 24 (N), so those cases will never be seen. These tests make a lot more sense in something like Android X that has more API levels to support.

Please keep us posted on when this code gets released into Android X, and at that time we can switch over to using it if the benefits outweigh the costs.

For better coordination, I'd suggest opening issues in the future to discuss code changes before implementing them. I know issues aren't currently enabled on this GitHub repo, so I'm going to check with the team to see what the best path for coordination is going forward.

Thanks again for your contributions!

Thanks for your advice. I've learned a lot from that.

I've made a tidy PendingIntentCompat specific for this project in the last commit which deleted the useless test and uncalled function. Also fix the problem that the test can't run.

I agree with your opinions on opening issues. Although this project can create issue here https://developer.android.com/guide/topics/sensors/gnss, it seems that we could just receive feedback and couldn't discuss here. So, it's not convenient. Looking forward to your good news!

[adaext]

I agree with @barbeau that we should not add code that is not used to the App. This will increase the code size and bring maintenance burden. You can keep the methods that are used and needed. Regarding adding this to Android-X, this is a very good idea. Let me know if you want me to connect you to the right people.

Thanks for your advice, I've commited a new version as you requested.

[barbeau]

Thanks @adaext for reducing the scope of the PR to code that will be used in this project.

Please see my comments in-line below.

[barbeau]

Could you use Expresso instead of Robolectric?

https://developer.android.com/training/testing/espresso/intents

I'd prefer to avoid external testing libraries unless absolutely necessary. I've opened PR #38 to add CI with multiple API levels that we can test on going forward. After that PR is merged, please merge master into this branch so the tests run on CI.

Also, my preference would be that all new classes be implemented in Kotlin instead of Java. See other comment for a way to do a quick conversion.

[adaext]

Could you use Expresso instead of Robolectric?

https://developer.android.com/training/testing/espresso/intents

I'd prefer to avoid external testing libraries unless absolutely necessary. I've opened PR #38 to add CI with multiple API levels that we can test on going forward. After that PR is merged, please merge master into this branch so the tests run on CI.

Also, my preference would be that all new classes be implemented in Kotlin instead of Java. See other comment for a way to do a quick conversion.

Thanks for your advice. Robolectric is very lightweight compared to Expresso. My class is simple enough and does not require too complicated tests. I think it may easier to use Robolectric.

[barbeau]

My issue with Robolectric is that it doesn't actually run on Android platform code, so you're not testing on an actual Android implementation. As a result I've preferred instrumented tests and have used Robolectric only as a method of last resort (e.g., when performance is critical, instrumented tests can't run on emulators/CI). GnssLogger will eventually have other instrumented tests so adding one more here for the PendingIntent doesn't add significant overhead.

It seems like the AndroidX library also prefers instrumented tests to Robolectric.

If you take a look, most of the unit tests for the AndroidX core library are here in androidTest:
https://github.com/androidx/androidx/tree/androidx-main/core/core/src/androidTest/java/androidx/core

...not in test using Robolectric on the JVM:
https://github.com/androidx/androidx/tree/androidx-main/core/core/src/test/java/androidx/core

If you look at the Robolectric tests, they are annotated with @DoNotInstrument, which means they exist there for a reason.

I also don't think you need full blown Expresso here - if you take a look at some of the Android X instrumented tests you'll get an idea of how you could mirror that approach with the PendingIntent.

[adaext]

My issue with Robolectric is that it doesn't actually run on Android platform code, so you're not testing on an actual Android implementation. As a result I've preferred instrumented tests and have used Robolectric only as a method of last resort (e.g., when performance is critical, instrumented tests can't run on emulators/CI). GnssLogger will eventually have other instrumented tests so adding one more here for the PendingIntent doesn't add significant overhead.

It seems like the AndroidX library also prefers instrumented tests to Robolectric.

If you take a look, most of the unit tests for the AndroidX core library are here in androidTest: https://github.com/androidx/androidx/tree/androidx-main/core/core/src/androidTest/java/androidx/core

...not in test using Robolectric on the JVM: https://github.com/androidx/androidx/tree/androidx-main/core/core/src/test/java/androidx/core

If you look at the Robolectric tests, they are annotated with @DoNotInstrument, which means they exist there for a reason.

I also don't think you need full blown Expresso here - if you take a look at some of the Android X instrumented tests you'll get an idea of how you could mirror that approach with the PendingIntent.

I agree with that instrumented tests are needed in this project. However, I don't know how to apply it on PendingIntentCompat. Because we can't get the parameters information directly through creating PendingIntent object. https://github.com/androidx/androidx/blob/androidx-main/core/core/src/androidTest/java/androidx/core/app/DialogCompatTest.java. However, shadow of Robolectirc could help us get those information.

[barbeau]

Could you please move this class to a new .util package and convert it to Kotlin? You can do this easily within Android Studio by right-clicking on the file and selecting "Convert Java file to Kotlin file". I'd prefer all new classes be written in Kotlin.

[adaext]

Could you please move this class to a new .util package and convert it to Kotlin? You can do this easily within Android Studio by right-clicking on the file and selecting "Convert Java file to Kotlin file". I'd prefer all new classes be written in Kotlin.

Ok, thanks

[adaext]

Could you please move this class to a new .util package and convert it to Kotlin? You can do this easily within Android Studio by right-clicking on the file and selecting "Convert Java file to Kotlin file". I'd prefer all new classes be written in Kotlin.

Ok, thanks

I've converted this class to Kotlin. But I found that the previous integrated google-java-format tool can't format Kotlin file. I think it's better to integrate Facebook Kotlin formatter into gradle, which is based on google-java-format. https://github.com/facebookincubator/ktfmt.

[barbeau]

Good point, we'll need a separate agreed-upon formatter for a Kotlin style.

The canonical style references seem to be:

• Android team - https://developer.android.com/kotlin/style-guide
• Kotlin language - https://kotlinlang.org/docs/coding-conventions.html

From a brief search, though, Google and Jetbrains don't seem to have their own standalone Kotlin formatting tool (they only have IntelilJ/Android Studio itself).

These seem to be the top two 3rd party contenders:

• https://github.com/facebookincubator/ktfmt
• https://github.com/pinterest/ktlint

Both seem to be actively maintained.

Let's leave this for a separate PR - I'll do some asking around to see what's preferred.

[barbeau]

For future reference, here's the PR for this same feature on Android X:
androidx/androidx#433

[mohammedkhider]

I still feel it might be too much to have a separate class within GnssLogger just for the purpose of handing PendingIntent mutability. On the other hand, I totally agree on moving the class to AndroidX and definitely would like to help in that.
@adaext let me sync with you on this.

[adaext]

I still feel it might be too much to have a separate class within GnssLogger just for the purpose of handing PendingIntent mutability. On the other hand, I totally agree on moving the class to AndroidX and definitely would like to help in that. @adaext let me sync with you on this.

Ok, thanks!

[CUMTdestiny]

你好，邮件已收到。