Skip to content

Conversation

@SowmiaS
Copy link
Contributor

@SowmiaS SowmiaS commented Apr 20, 2022

Part of #12372

Description

The Third PR for the task of refactoring the AccountSettingsFragment to MVVM following the reactive approach using Flow. This PR implements the LifeCycleOwner for PreferenceFragment.

Why

Since android.preference.PreferenceFragment doesn't have support to observe Flows and LiveData. Implementing reactive approach seems impossible.

Possible Solutions

  1. App has to be migrated to Androidx Preference Library.
  2. Implement LifeCycleOwner interface to support observing Flows and LiveData.

Reason for choosing Approach 2 - Implement LifeCycleOwner interface

Approach-1 (migrating to Androidx preference library) requires code changes in all Preference related classes. This can be done as a separate task.
So I have created a separate class, which implements LifeCycleOwner interface to support observing Flows and LiveData.

Note: This is just a temporary solution. Once the app is migrated to Androidx Preference Library, this class can be deleted.

Regression Notes

No Potential impact on screens as the implementation is not consumed by AccountSetingsFragment.

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

@ashiagr
Copy link
Contributor

ashiagr commented Jun 2, 2022

@SowmiaS 👋

Can you merge latest trunk to this PR branch? Ping me once it is done so that I can start reviewing it.

Thank you!

@SowmiaS SowmiaS closed this Jun 12, 2022
@SowmiaS SowmiaS force-pushed the issue/12372_implement_lifecycleowner_for_preferencefragment branch from 25a7208 to 8a601be Compare June 12, 2022 16:50
… functions for deprecated PreferenceFragment
@SowmiaS SowmiaS reopened this Jun 13, 2022
@SowmiaS SowmiaS marked this pull request as ready for review June 13, 2022 06:10
@SowmiaS
Copy link
Contributor Author

SowmiaS commented Jun 13, 2022

@ashiagr Sorry for the delay. The PR is ready for review now.
Thanks 🙂

@AjeshRPai AjeshRPai self-requested a review June 14, 2022 06:54
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jun 14, 2022

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

Copy link
Contributor

@AjeshRPai AjeshRPai left a comment

Choose a reason for hiding this comment

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

@SowmiaS I have looked at the PreferenceFragmentLifeCycleOwner. Code wise it looks fine to me 👍🏼 . As there aren't user-facing changes, I look forward to the next PR to see how AccountSettingsFragment will use this class. Thanks for working on this. I have initiated the CI to run on this PR. I will merge the PR after the CI completes successfully. 👍🏼

@AjeshRPai AjeshRPai merged commit cf5193f into wordpress-mobile:trunk Jun 14, 2022
@SowmiaS
Copy link
Contributor Author

SowmiaS commented Jun 15, 2022

@AjeshRPai Thank you for taking your time to review this PR. 🙌

@AliSoftware
Copy link
Contributor

AliSoftware commented Jun 27, 2022

Thanks again for this PR @SowmiaS 🎉

For information, your fix has been included in the version 20.2-rc-1 that we are now sending to beta-testing phase before releasing it to the public in two weeks. Be sure to watch https://make.wordpress.org/mobile/ for a soon-to-be published post about the beta-testing of WPAndroid 20.2, to help us test this version and your fix that shipped in it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants