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

Add immutable flag to pending intents #593

Merged
merged 1 commit into from
Apr 2, 2021
Merged

Add immutable flag to pending intents #593

merged 1 commit into from
Apr 2, 2021

Conversation

MiSikora
Copy link
Contributor

📄 Context

Android 31 requires to specify mutability of pending intents.

📝 Changes

I added mutability flag to our pending intents.

🚫 Breaking

No.

🛠️ How to test

Not sure, run on Android 31?

⏱️ Next steps

Either cherry–pick it to 3.x or plan release of 4.x soonish.

)
return NotificationCompat.Action(R.drawable.chucker_ic_delete_white, clearTitle, intent)
}

fun dismissNotifications() {
notificationManager.cancel(TRANSACTION_NOTIFICATION_ID)
}

private fun immutableFlag() = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need this check?
Flags are generally forwards compatible.
During compilation constant will be inlined and in runtime on pre-M those bits won't be read.
Warning will be generated about using constants from higher API levels than min SDK but in this case such usage is legitimate IMHO.

Copy link
Contributor Author

@MiSikora MiSikora Mar 28, 2021

Choose a reason for hiding this comment

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

I'm not sure what are you proposing. I don't want IDE to highlight this and Lint to fail. Do you suggest to add @SuppressLint("InlinedApi")? R8 can remove this condition anyway with -assumevalues. Then there is dex2oat and finally ART itself.

Though I don't mind adding annotation. Depends on what others think.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you suggest to add @SuppressLint("InlinedApi")

Exactly.

R8 can remove this condition

True, but not everyone use R8.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, but not everyone use R8.

I would say that if someone does not use R8 then they don't care about optimizations anyway. :)

Copy link
Member

Choose a reason for hiding this comment

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

I'd be more for having the explicit if-then-else. It's more aligned with the rest of the codebase and makes the code more explicit.

@MiSikora MiSikora merged commit 244ca14 into develop Apr 2, 2021
@MiSikora MiSikora deleted the mutability-flag branch April 2, 2021 17:10
vbuberen pushed a commit that referenced this pull request Jun 26, 2021
@vbuberen vbuberen mentioned this pull request Jun 26, 2021
vbuberen added a commit that referenced this pull request Jun 28, 2021
* Update all dependencies and configs, update code for Detekt

* Prepare for next release (#494)

* Prepare for next release
* Update workflows and readme to mirror latest changes

* Update publishing action name (#495)

* Fixed typo on DialogData (#501)

* Update README.md (#505)

fix the typo

* Fix setting request body plain text in transaction (#538)

* Fix setting request body plain text in transaction

* Add test for plain text request body

* Switch to CircularProgressIndicator

* Switch to Activity Result API

* Add immutable flag to pending intents (#593)

* Update Github Actions workflows to match latest ones

* Bump version

* Resolve lint issues

* Remove test using newer OkHttp API

* Remove breaking change with BuildConfig removal

* Bump kotlinVersion from 1.5.10 to 1.5.20 (#639)

Bumps `kotlinVersion` from 1.5.10 to 1.5.20.

Updates `kotlin-gradle-plugin` from 1.5.10 to 1.5.20
- [Release notes](https://github.com/JetBrains/kotlin/releases)
- [Changelog](https://github.com/JetBrains/kotlin/blob/v1.5.20/ChangeLog.md)
- [Commits](JetBrains/kotlin@v1.5.10...v1.5.20)

Updates `kotlin-stdlib` from 1.5.10 to 1.5.20
- [Release notes](https://github.com/JetBrains/kotlin/releases)
- [Changelog](https://github.com/JetBrains/kotlin/blob/v1.5.20/ChangeLog.md)
- [Commits](JetBrains/kotlin@v1.5.10...v1.5.20)

---
updated-dependencies:
- dependency-name: org.jetbrains.kotlin:kotlin-gradle-plugin
  dependency-type: direct:production
  update-type: version-update:semver-patch
- dependency-name: org.jetbrains.kotlin:kotlin-stdlib
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Co-authored-by: Nicola Corti <[email protected]>
Co-authored-by: Okan AYDIN <[email protected]>
Co-authored-by: Michał Sikora <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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.

4 participants