Update deprecated Parcel functions by irfano · Pull Request #18068 · wordpress-mobile/WordPress-Android

irfano · 2023-03-08T20:42:43Z

This PR is part of a parent PR to update compileSdk to 33.

androidx.core was updated to 1.9.0 to be able to use ParcelCompat.readParcelable and ParcelCompat.readList functions.

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

Regression Notes

Potential unintended areas of impact
Notifications might be affected.
What I did to test those areas of impact (or what existing automated tests I relied on)
I tested notifications manually on devices with Android 13 and lower version.
What automated tests I added (or what prevented me from doing so)
N/A

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.

peril-wordpress-mobile · 2023-03-08T20:42:48Z

	Warnings
⚠️	PR is not assigned to a milestone.

Generated by 🚫 dangerJS

wpmobilebot · 2023-03-08T21:01:23Z

📲 You can test these changes on WordPress by downloading wordpress-installable-build-pr18068-b5bc515.apk

	💡 Scan this QR code with your Android phone to download and install the APK directly on it.
	App	`WordPress`
	Build Flavor	`Jalapeno`
	Build Type	`Debug`
	Commit	`b5bc515`

Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

wpmobilebot · 2023-03-08T21:01:24Z

📲 You can test these changes on Jetpack by downloading jetpack-installable-build-pr18068-b5bc515.apk

	💡 Scan this QR code with your Android phone to download and install the APK directly on it.
	App	`Jetpack`
	Build Flavor	`Jalapeno`
	Build Type	`Debug`
	Commit	`b5bc515`

Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

ParaskP7

👋 @irfano !

I have reviewed and tested this PR as per the instructions, everything works as expected, good job! 🌟 🌟 🌟

FYI: In addition to the basic smoke test, I did some target testing, see below:

Media Screen: Because of the MediaItem.kt change.
Stats Screen: Because of the SeelctedDateProvider.kt change.
Notifications Screen & Functionality: Because of the AndroidManifest.xml change.

I can verify that everything works as expected! 🚀

Added android.permission.POST_NOTIFICATIONS to fix the build error.

Question (❓): How did the build failed, I tried removing this permission and running assembleJetpackWasabiDebug in order to notice this build error but couldn't identify one (nor a warning). 🤔

Can you guide me here please? 🙏

...but notifications still works on Android 13 and lower versions.

This is true, I tested this by creating a new post and then seeing the notification showing right after, that is, even though I clicked the Don't Allow button when the Notification Permission dialog appeared after a clean install of the app.

ParaskP7 · 2023-03-09T10:59:27Z

FYI: The only thing that doesn't make me feel comfortable with this change is the fact that the 1.9.0 transitively updates Kotlin from 1.6.21 to 1.7.10 (see here). 😞

PS: I was hoping to avoid such a transitive update and do such an update via #17563 . But, I guess we are now reaching a deadlock where #17563 needs #17790 and #17790 can't be completed with the the 1.9.0 update. 🤷

Or maybe, we could skip the 1.9.0 update in this PR, just to be on the safe side and add create our own compat extension functions, just like you did with #18061. 🤔

Wdyt @irfano ?

This reverts commit ff3699a.

This reverts commit 86dd01a.

This reverts commit d33a199.

irfano · 2023-03-09T15:01:13Z

FYI: In addition to the basic smoke test, I did some target testing, see below:

Thank you for the additional testing, @ParaskP7. ❤️

Question (❓): How did the build failed, I tried removing this permission and running assembleJetpackWasabiDebug in order to notice this build error but couldn't identify one (nor a warning). 🤔

Can you guide me here please? 🙏

It was showing a warning when running ./gradlew lintWordPressVanillaRelease but not blocking the build since allWarningsAsErrors is disabled temporarily in the feature branch.

Or maybe, we could skip the 1.9.0 update in this PR, just to be on the safe side and add create our own compat extension functions, just like you did with #18061. 🤔

I reverted all changes and created new functions in CompatExtensions to be on the safe side.
Can you please handle updating comments on related issues and open a new PR to update androidx.core to 1.9.0?

…-for-android13 # Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/stats/refresh/lists/sections/granular/SelectedDateProvider.kt # WordPress/src/main/java/org/wordpress/android/util/extensions/CompatExtensions.kt

irfano · 2023-03-09T15:27:36Z

This is true, I tested this by creating a new post and then seeing the notification showing right after, that is, even though I clicked the Don't Allow button when the Notification Permission dialog appeared after a clean install of the app.

You saw the notification on the Notifications tab in the app, right?
The "Don't allow" button on the notification permission dialog prevents notification alerts from being displayed on the device, but they can still appear on the Notifications tab of the app.

ParaskP7 · 2023-03-09T15:33:33Z

👋 @irfano and thank you for the update! ❤️ 🙇 🚀

It was showing a warning when running ./gradlew lintWordPressVanillaRelease but not blocking the build since allWarningsAsErrors is disabled temporarily in the feature branch.

I see, I see, so it was a warning then not an error, it just this warnings was failing the build due to allWarningsAsErrors, got it! 🙏

Btw, thanks for reverting this change, I agree with that too, as it might be better to add this commit when we re-enabled allWarningsAsErrors on the parent PR. 💯

I reverted all changes and created new functions in CompatExtensions to be on the safe side.

This is great, thanks for doing that, I agree, staying on the safe side might be wise atm! 💯

FYI: I went on another round of reviewing and testing, everything looks LGTM and works as expected! 🌟 🌟 🌟

It is a ✅ from my side, let's merge! 🚀

Can you please handle updating comments on related issues and open a new PR to update androidx.core to 1.9.0?

Done! 🎯

FYI: I added an UPDATE section on this comment of mine.

ParaskP7 · 2023-03-09T15:40:34Z

You saw the notification on the Notifications tab in the app, right?
The "Don't allow" button on the notification permission dialog prevents notification alerts from being displayed on the device, but they can still appear on the Notifications tab of the app.

I actually saw the notification alerts being displayed even on the device, not only on the Notifications tab, which would have been displayed anyway of course.

That is with me clicking Don't allow, yes... 🤷 I am not sure if I am missing something. 🤔

Can you give me an example of a specific notification that during your testing, when you click Don't allow, on an Android 13 device, that notification is NOT shown, that is, it not being popping-up in your device notifications panel, versus it being shown when you click Allow instead. 🙏

irfano · 2023-03-09T16:06:10Z

You saw the notification on the Notifications tab in the app, right?
The "Don't allow" button on the notification permission dialog prevents notification alerts from being displayed on the device, but they can still appear on the Notifications tab of the app.

I actually saw the notification alerts being displayed even on the device, not only on the Notifications tab, which would have been displayed anyway of course.

That is with me clicking Don't allow, yes... 🤷 I am not sure if I am missing something. 🤔

Can you give me an example of a specific notification that during your testing, when you click Don't allow, on an Android 13 device, that notification is NOT shown, that is, it not being popping-up in your device notifications panel, versus it being shown when you click Allow instead. 🙏

I have tested the case and here are the instructions:

Perform a clean installation of JP.
Tap on "Don't allow" when the notification permission dialog appears.
Log in to the app.
Send a comment to my site from the web.
No notifications appeared on the device.

After tapping the "Don't allow" button, the notification permission should be off in the app settings.

ParaskP7 · 2023-03-09T16:23:18Z

👋 @irfano !

I have tested the case and here are the instructions:

Ah, you are right, it was my bad, I am having all 3 variations of the Jetpack app on my testing phone, production (vanilla), beta (wasabi) and alpha (jalapeno). The production Jetpack app was logged-in with my user and was receiving the notification, and I, without noticing that the icon on that notification was the production Jetpack icon (not the beta Jetpack icon), incorrectly assumed that notification was working even when we are clicking Don't allow... 😅 🚏 🚌

I tested it again just now, just to be on the safe side, with both, clicking Don't allow and Allow, sending the notification as per your instructions and I can verify I get the same behaviour to your testing, as expected, obviously! 😅 🛌 😴

Thank you for making me realize this silly testing mistake of mine! 🙇 😅 💤

irfano · 2023-03-09T16:38:10Z

Ah, you are right, it was my bad, I am having all 3 variations of the Jetpack app on my testing phone, production (vanilla), beta (wasabi) and alpha (jalapeno). The production Jetpack app was logged-in with my user and was receiving the notification, and I, without noticing that the icon on that notification was the production Jetpack icon (not the beta Jetpack icon), incorrectly assumed that notification was working even when we are clicking Don't allow... 😅 🚏 🚌

I tested it again just now, just to be on the safe side, with both, clicking Don't allow and Allow, sending the notification as per your instructions and I can verify I get the same behaviour to your testing, as expected, obviously! 😅 🛌 😴

Thank you for making me realize this silly testing mistake of mine! 🙇 😅 💤

It's good to test all cases. 😄
It looks like we're good to merge now. 🚀

irfano added 3 commits March 8, 2023 23:25

Update androidx.core version to 1.9.0

ff3699a

Add android.permission.POST_NOTIFICATIONS

86dd01a

Use ParcelCompat for Android 13 changes

d33a199

irfano added [Type] Task Android 13 labels Mar 8, 2023

irfano requested a review from ParaskP7 March 8, 2023 20:42

irfano mentioned this pull request Mar 8, 2023

Parent PR for updating compileSdk 33 #17947

Merged

8 tasks

ParaskP7 self-assigned this Mar 9, 2023

ParaskP7 mentioned this pull request Mar 9, 2023

[Dependency Updates] Update androidxCoreVersion to 1.8.0 #17691

Merged

11 tasks

ParaskP7 approved these changes Mar 9, 2023

View reviewed changes

irfano added 6 commits March 9, 2023 16:03

Revert "Update androidx.core version to 1.9.0"

d98d281

This reverts commit ff3699a.

Revert "Add android.permission.POST_NOTIFICATIONS"

28fff0b

This reverts commit 86dd01a.

Revert "Use ParcelCompat for Android 13 changes"

4047e5a

This reverts commit d33a199.

Add Android 13 compatibility functions for Parcel

64f591a

Update deprecated Parcel functions

21ad209

Add comments to functions in CompatExtensions

b194387

irfano requested a review from ParaskP7 March 9, 2023 15:26

irfano merged commit 01736fb into feature/update-compile-sdk-33 Mar 9, 2023

irfano deleted the update-parcelcompat-for-android13 branch March 9, 2023 16:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update deprecated Parcel functions#18068

Update deprecated Parcel functions#18068
irfano merged 10 commits intofeature/update-compile-sdk-33from
update-parcelcompat-for-android13

irfano commented Mar 8, 2023 •

edited

Loading

Uh oh!

peril-wordpress-mobile bot commented Mar 8, 2023

Uh oh!

wpmobilebot commented Mar 8, 2023 •

edited

Loading

Uh oh!

wpmobilebot commented Mar 8, 2023 •

edited

Loading

Uh oh!

ParaskP7 left a comment

Uh oh!

ParaskP7 commented Mar 9, 2023

Uh oh!

irfano commented Mar 9, 2023 •

edited

Loading

Uh oh!

irfano commented Mar 9, 2023

Uh oh!

ParaskP7 commented Mar 9, 2023

Uh oh!

ParaskP7 commented Mar 9, 2023

Uh oh!

irfano commented Mar 9, 2023

Uh oh!

ParaskP7 commented Mar 9, 2023

Uh oh!

irfano commented Mar 9, 2023 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

irfano commented Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Regression Notes

Uh oh!

peril-wordpress-mobile bot commented Mar 8, 2023

Uh oh!

wpmobilebot commented Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ParaskP7 left a comment

Choose a reason for hiding this comment

Uh oh!

ParaskP7 commented Mar 9, 2023

Uh oh!

irfano commented Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

irfano commented Mar 9, 2023

Uh oh!

ParaskP7 commented Mar 9, 2023

Uh oh!

ParaskP7 commented Mar 9, 2023

Uh oh!

irfano commented Mar 9, 2023

Uh oh!

ParaskP7 commented Mar 9, 2023

Uh oh!

irfano commented Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

irfano commented Mar 8, 2023 •

edited

Loading

wpmobilebot commented Mar 8, 2023 •

edited

Loading

wpmobilebot commented Mar 8, 2023 •

edited

Loading

irfano commented Mar 9, 2023 •

edited

Loading

irfano commented Mar 9, 2023 •

edited

Loading