Skip to content

Conversation

irfano
Copy link
Member

@irfano irfano commented Jan 29, 2023

Fixes #17790

This PR updates compileSdkVersion to 33 and fixes errors and warnings caused by the update.

This contains a lot of changes, but they are small and similar. I could publish this via multiple PRs, but multiple PRs would create a burden of merging in order and creating multiple branches based on others. I believe it won't be hard to review this if you follow the commits.

I've added @ravishanker, but he'll be on support rotation next week. I'd appreciate it if you could review, @ParaskP7.

Commits

  1. 61992b1: compileSdkVersion was updated to Android 33.
  2. 9446c7f: onBackPressed was deprecated on Android 13. It's updated only on Kotlin files. Because only errors on Kotlin files prevent the build. onBackPressed on Java files can be updated while converting them to Kotlin.
  3. a8d466a: CompatExtensions was added to use in the next commit.
  4. d8deb99: Some functions of Bundle and Intent was deprecated. To update them for Android 13, there are functions in IntentCompat and BundleCompat but they're available on the 1.10.0-alpha version which is not stable yet. I used CompatExtensions that I created to update deprecated functions.
  5. b767c46: Updated some AnimatorListener parameters which are not nullable anymore.
  6. 44dfcad: Updated some MenuItem parameters which are not nullable anymore.
  7. cf40f4e: Updated some SimpleOnGestureListener parameters which are not nullable anymore.
  8. 73662bd: Updated androidx.core version to 1.9.0 because we'll need new ParcelCompat functions in the next commit.
  9. 22a3914: Use ParcelCompat for deprecated functions.
  10. 1287b22: Update deprecated PackageManager functions.
  11. 37a3113: Added android.permission.POST_NOTIFICATIONS to fix the build error. Now NotificationManagerCompat.from(context).notify(id, notification) shows the error "Call requires permission which may be rejected by user: code should explicitly check to see if permission is available (with checkPermission) or explicitly handle a potential SecurityException" but notifications still works on Android 13 and lower versions. Runtime permission for notifications #17714 will be handled later.
  12. 995e447: Fixed unit tests by updating deprecated functions and making superclass of T nullable in CompatExtensions. This also fixes possible null exceptions on runtime.

Note
I also took action for some warnings from Android Studio, like removing redundant SAM-constructor if the warning is just inside the function I work on.

To test:
Being able to build and basic smoke test should be enough.

Regression Notes

  1. Potential unintended areas of impact
    Since this touches a lot of classes, every screen can be impacted, but no change is expected on runtime. But especially back gestures, passing data between screens and notifications.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    I tested the back gesture manually to verify it's still working.

  3. What automated tests I added (or what prevented me from doing so)
    None. This is just a library and deprecated functions update.

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@wpmobilebot

This comment was marked as off-topic.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 29, 2023

WordPress📲 You can test these changes on WordPress by downloading wordpress-installable-build-pr17842-34085d4.apk
💡 Scan this QR code with your Android phone to download and install the APK directly on it.
AppWordPress
Build FlavorJalapeno
Build TypeDebug
Commit34085d4
Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 29, 2023

Jetpack📲 You can test these changes on Jetpack by downloading jetpack-installable-build-pr17842-34085d4.apk
💡 Scan this QR code with your Android phone to download and install the APK directly on it.
AppJetpack
Build FlavorJalapeno
Build TypeDebug
Commit34085d4
Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

These functions were deprecated on Android 13.
Fix failing LoginNoSitesViewModelTest
@ParaskP7 ParaskP7 self-assigned this Jan 30, 2023
Copy link
Contributor

@ParaskP7 ParaskP7 left a comment

Choose a reason for hiding this comment

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

👋 @irfano !

First of all, a big bravo for working on this compileSdkVersion = 33 upgrade and creating this PR, this work of yours will unblock many things for us, and it wasn't easy, I bet! 💯 🙇 ❤️


This contains a lot of changes, but they are small and similar. I could publish this via multiple PRs, but multiple PRs would create a burden of merging in order and creating multiple branches based on others. I believe it won't be hard to review this if you follow the commits.

Ah, actually, I would have still advised you to create multiple PRs. You will understand why I am saying that after you read through my code review change requests and comments below.

FYI: It took me almost a day to review this PR and more so to gather some feedback for you, that is, in a way that can make it easier for you to apply any of those changes I identified and can recommend to you. Also, I created some code batches for you, just to make it that bit easier for you to review those and apply them.


Another FYI, the fact that some commits, especially the big ones (9446c7f and d8deb99) weren't split into multiple commits, made it a bit more difficult for me to review this PR, so apologies for the delay.

  • For example, for the SiteCreationActivity change, included in 9446c7f, you could have split the changes there into two commits, one for the SAM changes, and one for the main onBackPressed change.
  • Another example, for the DomainSuggestionsFragment change, included in d8deb99, you could have split the changes there into 2 commits, one for the DI changes, and one for the main CompatExtensions change.

For the future, I would really recommend that kind of changes to be split into multiple commits, just so that to keep the main commit on focus on the actual change that warranties a bit more thoughtful code review in case things got slipped.


I reviewed this PR but not tested it as much as I would have liked. But, I did that on purpose, this is because:

  1. Ideally, I would like us to bring a QE to the rescue here and have them test this PR thoroughly as well. For example, I did the same with the AndroidX Core dependency updates (see pdcxQM-1HP-p2) and I think this update deserves the same level of testing before merging. There are lots of changes here and even if they don't seem to big, they have the potential to break things.
  2. I have some a few code review change requests for you. I would like them to be handled first before I can begin the testing part on my side.

Request Changes:

  1. Warning (⚠️): For the d8deb99 change, I suggest redoing that change, almost reverting half of it. Instead, I recommend explicitly adding the requireNotNull(...) on all the changes that require a non-null value, that is, instead of guarding that and checking for nullability before proceeding forward. This is because by guarding that line and then checking whether that value is not null, instead of crashing the app, the flow will continue, but that will create more confusion and unexpected behavior later on. It is better to crash the app at that point, just like it would have crashed even before this update anyway. Wdyt? PS: I have created a patch for you on that just to make those change a bit more easier on you.

[Compile_SDK_33]_Compat_Extensions_Related_Code_Review_Changes.patch

  1. Suggestion (💡) I also recommend you add the same CompatExtensions.kt file on the image-editor lib module and then use that on CropViewModel.kt and PreviewImageFragment.kt, the same way you did for the WordPress module. I understand that you didn't do it because you didn't want to create a duplication of extension files, but I believe it is better you do that now and then when the 1.10.0 becomes ready, to delete both files and use those IntentCompat and BundleCompat instead. Thus, I believe it is better to have those 2 temp extension files for now, as this will mean that when 1.10.0 becomes ready there would be no code changes, nowhere, only import changes. Wdyt? PS: The patch above includes this suggestion change as well.
  2. Suggestion (💡): Consider explicitly setting the type on all the CompatExtensions.kt function calls, that is, instead of let it be referred sometimes. PS: The patch above includes this suggestion change as well.
  3. Warning (⚠️): I did search for all usages of onBackPressed in order to verify that all of them have been updated away from their deprecated usage and into the new recommended custom back navigation, using the onBackPressedDispatcher. I found a few instances where this wasn't done and I recommend those Kotlin related activities and fragments to get that update too, no matter if the build doesn't complain about it (for an unknown to me reason). I also recommend we do the same change on all the Java related activities and fragments, no matter if our build succeeds without failing, just because it only complains on Kotlin classes, the idea is the same, later on, newer APIs and updates would no longer allow for these deprecated calls. Thus, if we can update them now, we should do that, especially given the fact that there are not that many such Java classes anyway, about 10 of them. Wdyt? PS: I have created a patch for you on that just to make those change a bit more easier on you, excluding the Java related changes.

[Compile_SDK_33]_On_Back_Pressed_Related_Code_Review_Changes.patch

  1. Suggestion (💡): Also, I recommend refactor some classes that are using the old way of utilizing the onBackPressedDispatcher callback, like the LoginNoSitesFragment class. PS: The patch above includes this suggestion change as well.
  2. Suggestion (💡): Consider reusing the PackageManagerWrapper.getPackageInfo(...) helper function instead of duplicating this logic in multiple places. I also suggest creating another such PackageManagerWrapper.getActivityInfo(...) helper function and reusing that were necessary. Wdyt? PS: I have created a patch for you on that just to make those change a bit more easier on you.

[Compile_SDK_33]_Package_Manager_Related_Code_Review_Changes.patch


I'll avoid writing any more comments for now, that is, until we discuss all the above first.

PS: Apologies for the wall of text and for adding lots on your plate before giving the 🟢 to merge this to trunk. But, I do believe this it is better for us to be careful with this change and invest some extra time to make sure this change is (a.) safe for merging and (b.) includes all of the necessary updates. I am here to support you on all that, all the way, so feel free to reach out for help with whatever you might need. 💯

currentIcon?.let {
// must mutate the drawable to avoid affecting other instances of it
val icon = menuItem.icon.mutate()
val icon = currentIcon.mutate()
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor (🔍): Consider using it.mutate instead.

val timePickerDialog = TimePickerDialog(
context,
OnTimeSetListener { _, selectedHour, selectedMinute ->
{ _, selectedHour, selectedMinute ->
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor (🔍): Consider making this lambda a one-liner.

@ravishanker
Copy link
Contributor

ravishanker commented Jan 31, 2023

I could publish this via multiple PRs, but multiple PRs would create a burden of merging in order and creating multiple branches based on others.

Enormous effort @irfano 👏

Is this supposed to go on a separate branch at first, not on the trunk direct!

Would it compile though, if it is split into multiple PRs (As @ParaskP7 suggested), unless the changes required are made first in separate PRs and then upgrade the compileSdkVersion at last.