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

ViewBinding: Remove Kotlin Android Extensions (Replace with Kotlin Parcelize) #14902

Merged
merged 6 commits into from
Jun 23, 2021

Conversation

ParaskP7
Copy link
Contributor

@ParaskP7 ParaskP7 commented Jun 23, 2021

Parent #14845

The kotlin-android-extensions plugin is no longer necessary and it is preventing us from updating to a newer gradle version. @oguzkocer and @malinajirka I am adding you as the main reviewers since you two are also working on merging the related stories-android draft PR, which (kind of) depends on this PR.

This PR is part of a larger effort to replace synthetic accessors within WPAndroid and is split into three parts:

  • Replace the kotlin-android-extensions plugin with kotlin-parcelize for the editor library.
  • Replace the kotlin-android-extensions plugin with kotlin-parcelize for the image-editor library.
  • Replace the kotlin-android-extensions plugin with kotlin-parcelize for the WordPress module.

Changes in this PR, for each part above:

  • Replace kotlin-android-extensions plugin with kotlin-parcelize.
  • Update deprecated kotlinx.parcelize.Parcelize import with kotlinx.parcelize.Parcelize
  • Suppress false positive ParcelCreator Lint error.

To test (for editor lib):

  • Launch the app.
  • Navigate to My Site -> Blog Posts.
  • Note that the various tabs with list of posts are shown as expected.
  • Navigate to FAB -> Blog post.
  • Note that the draft post has been successfully created and that it is shown as expected.

To test (for image-editor lib):

  • Launch the app.
  • Navigate to My Site -> Blog Posts -> FAB -> Blog post.
  • Type a title and some content.
  • Click on the + 🔵 button on the bottom left, on top of the keyboard, to then select an Image block.
  • Click ADD IMAGE -> Choose from device -> Select an image of your choice -> Click the Confirm tick on the top right of the screen.
  • After the image has been loaded, click the Media options icon on the top right of the image -> Select Edit.
  • The PreviewImageFragment edit screen should be launched.
  • Note that the preview image shows as expected.

To test (for WordPress module):

  • Launch the app.
  • Navigate to My Site -> Stats.
  • Note that the various tabs with list of stats are shown as expected.
  • PS: There have been a number of other changes that can be tested, but testing the Stats screen should be enough, since this will verify that all the other similar changes also work as expected. But in case you feel strongly about smoke testing all those other screens as well, please feel free to do it.

Regression Notes

  1. Potential unintended areas of impact

N/A

  1. What I did to test those areas of impact (or what existing automated tests I relied on)

Manually tested as described above in the To test sections.

  1. 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.

ParaskP7 added 6 commits June 23, 2021 13:00
Since this lib utilizes the '@parcelize' annotation (ie.
'GutenbergPropsBuilder'), the 'kotlin-android-extensions' plugin cannot
be removed completely, but instead it needs to be replaced with the
'kotlin-parcelize' plugin.
Parcelize annotations from package 'kotlinx.android.parcel' are
deprecated. The newly 'kotlinx.parcelize' package is now the default and
as such the corresponding import have been updated accordingly.

Also, as part of this change the new 'ParcelCreator' error needs to be
suppressed since it is a false positive. Classes which use @parcelize
don't need to manually implement 'ParcelCreator' anymore.

Documentation: https://developer.android.com/kotlin/parcelize
Since this lib utilizes the '@parcelize' annotation (ie.
'PreviewImageFragment'), the 'kotlin-android-extensions' plugin cannot
be removed completely, but instead it needs to be replaced with the
'kotlin-parcelize' plugin.
Parcelize annotations from package 'kotlinx.android.parcel' are
deprecated. The newly 'kotlinx.parcelize' package is now the default and
as such the corresponding import have been updated accordingly.

Also, as part of this change the new 'ParcelCreator' error needs to be
suppressed since it is a false positive. Classes which use @parcelize
don't need to manually implement 'ParcelCreator' anymore.

Documentation: https://developer.android.com/kotlin/parcelize
Since this lib utilizes the '@parcelize' annotation (ie.
'SelectedDateProvider' on 'StatsActivity'), the
'kotlin-android-extensions' plugin cannot be removed completely, but
instead it needs to be replaced with the 'kotlin-parcelize' plugin.
Parcelize annotations from package 'kotlinx.android.parcel' are
deprecated. The newly 'kotlinx.parcelize' package is now the default and
as such the corresponding import have been updated accordingly.

Also, as part of this change the new 'ParcelCreator' error needs to be
suppressed since it is a false positive. Classes which use @parcelize
don't need to manually implement 'ParcelCreator' anymore.

Documentation: https://developer.android.com/kotlin/parcelize
@peril-wordpress-mobile
Copy link

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link

You can test the changes on this Pull Request by downloading the APK here.

Copy link
Contributor

@zwarm zwarm left a comment

Choose a reason for hiding this comment

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

Nice work @ParaskP7.
All requested testing looks good, plus I moved around the app and did some spot testing of other features. 🚀

@ParaskP7
Copy link
Contributor Author

👋 @zwarm !

All requested testing looks good, plus I moved around the app and did some spot testing of other features. 🚀

Thank you so much for doing some additional smoke testing of the other features, you rock! 🚀 🙇

@malinajirka
Copy link
Contributor

I've scanned through this PR and it LGTM. Great job! ;) So AFAIU we can merge this PR and then merge the stories PR and that's it right?

@oguzkocer
Copy link
Contributor

Thank you all for working on this! Since it's already approved and Jirka had a quick look as well, could we merge this in as soon as we can @ParaskP7 @zwarm?

@malinajirka Yes, I think the next step is to update stories-android. If you need any PR reviews, could you add @ParaskP7 & @zwarm since they have a lot more knowledge on the subject?

@zwarm
Copy link
Contributor

zwarm commented Jun 23, 2021

@oguzkocer @malinajirka
Yes, all is good. I'm going to merge this one and then you can merge the stories library.

Thanks everybody.

@zwarm zwarm merged commit d01c50b into develop Jun 23, 2021
@zwarm zwarm deleted the issue/14845-remove-kotlin-android-extensions branch June 23, 2021 19:21
@ParaskP7
Copy link
Contributor Author

👋 @malinajirka @oguzkocer @zwarm !

I've scanned through this PR and it LGTM. Great job! ;) So AFAIU we can merge this PR and then merge the stories PR and that's it right?

Great, thanks so much for going through the changes Jirka and for singing off on the merge! 🙏

Thank you all for working on this! Since it's already approved and Jirka had a quick look as well, could we merge this in as soon as we can @ParaskP7 @zwarm?

Great, thank you Oguz! I am seeing Annmarie did the merging honours, woohoo! 🎉

@malinajirka Yes, I think the next step is to update stories-android. If you need any PR reviews, could you add @ParaskP7 & @zwarm since they have a lot more knowledge on the subject?

👍

@oguzkocer @malinajirka
Yes, all is good. I'm going to merge this one and then you can merge the stories library.

Thank you Annmarie! 🥇

Thanks everybody.

💯 THIS ^ 🏅

ParaskP7 added a commit to wordpress-mobile/WordPress-FluxC-Android that referenced this pull request Jul 11, 2023
Release Notes: https://github.com/JetBrains/kotlin/releases/tag/v1.6.20

------------------------------------------------------------------------

This is a tmp solution which will be removed once the 'Catalog' related
Automattic/android-dependency-catalog#20 PR
gets ready for review and the CI is able to publish the new catalog.

------------------------------------------------------------------------

FYI: Instead of updating Kotlin to this newer '1.6.20' patch version, a
better alternative would have been to replace the deprecated
'kotlin-android-extensions' plugin with 'kotlin-parcelize'. However,
doing such a migration is not trivial and will require a lot of work.
For instance, one could search for all the 'kotlinx.android.synthetic'
and 'kotlinx.android.parcel' related imports. Those would need to be
replaced with 'ViewBinding' and 'kotlinx.parcelize' respectively.

As a reference see WPAndroid such issues and their multiple PRs:
- Replace Synthetic Accessors with ViewBinding #14845
wordpress-mobile/WordPress-Android#14845
- ViewBinding: Remove Kotlin Android Extensions (Replace with Kotlin
Parcelize) #14902
wordpress-mobile/WordPress-Android#14902
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants