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

[Preference Migration] Migrate Legacy Android Preference to AndroidX Preference #17962

Open
Tracked by #17551
ParaskP7 opened this issue Feb 15, 2023 · 10 comments · May be fixed by #19986
Open
Tracked by #17551

[Preference Migration] Migrate Legacy Android Preference to AndroidX Preference #17962

ParaskP7 opened this issue Feb 15, 2023 · 10 comments · May be fixed by #19986

Comments

@ParaskP7
Copy link
Contributor

As part of my previous work on #17215 and #17960 I keep stumbling on the fact that on WPAndroid, instead of using the AndroidX Preference library for anything settings related, it is still using the legacy and long deprecated Android Preference library.

⚠️ As per the Settings documentations, starting with Android 10 (API 29), the legacy android.preference library, referred to as the platform android.preference library, is deprecated. For consistent behavior across all devices it is recommended to use the androidx.preference library.

Also, in addition to the above, the WPAndroid preference configuration and usage just that bit more complicated. The complication comes from the fact that WPAndroid used both android.preference and androidx.preference (see #17215).

  • androidx.preference is used only for PreferenceManager related work, while
  • android.preference is used for everything else.

The above makes that bit more complicated to update the AndroidX Preference library to newer versions, and that is, because as one would need to know all the above before deciding towards the updating strategy.

For example, while working on #17960 I was tempted to also try and migrated at least one screen from android.preference and into androidx.preference. I tried with the seemingly simple AccountSettingsFragment.kt class, which is related to the very simple Account Settings screen, but without success, not at all. After a while, I realized that everything is interconnected in such a degree that it is impossible to complete this migration without affecting any other screen (more on that below).

Technical Details

While trying to migrate AccountSettingsFragment.kt class to androidx.preference, during my work on #17960 I ended up mapping down every usage of android.preference and anything that is depending on it, via extending those classes, or using those via composition.

Below is a summary of that, you can expand and see the details per class, as well as how to test each class:

1. AccountSettingsFragment.kt

Res: account_settings.xml

Extends from: PreferenceFragmentLifeCycleOwner.kt

Uses:

  • EditTextPreferenceWithValidation.java, which extends from SummaryEditTextPreference.java
  • DetailListPreference.java

To test:

  • Go to Me screen.
  • Click on the Account Settings button.
  • Verify that the Account Settings screen is displayed.
  • Click on each of the settings within the Account Settings screen and verify that every setting works as expected.
2. AppSettingsFragment.java

Res: app_settings.xml

Uses:

  • WPActivityUtils.java, which uses add/removeToolbarFromDialog(...)
  • WPSwitchPreference.java, which uses SummaryEditTextPreference.java
  • DetailListPreference.java
  • LearnMorePreference.java
  • WPPreference.java, which uses DetailListPreference.java`
  • WPPrefUtils.java

To test:

  • Go to Me screen.
  • Click on the App Settings button.
  • Verify that the App Settings screen is displayed.
  • Click on each of the settings within the App Settings screen and verify that every setting works as expected, including the inner settings like the Privacy Settings and Debug Settings screens.
3. SiteSettingsFragment.java

Res: site_settings.xml

Uses:

  • WPActivityUtils.java, which uses add/removeToolbarFromDialog(...)
  • WPSwitchPreference.java, which uses SummaryEditTextPreference.java
  • EditTextPreferenceWithValidation.java, which extends from SummaryEditTextPreference.java
  • SummaryEditTextPreference.java
  • DetailListPreference.java
  • LearnMorePreference.java
  • WPPreference.java, which uses DetailListPreference.java`
  • WPStartOverPreference.java, which extends from WPPreference.java
  • WPPrefUtils.java

To test:

  • While on the My Site/MENU tab.
  • Click on the Site Settings button.
  • Verify that the Site Settings screen is displayed.
  • Click on each of the settings within the Site Settings screen and verify that every setting works as expected.
4. JetpackSecuritySettingsFragment.java

Res: jetpack_settings.xml

Uses:

  • WPSwitchPreference.java, which uses SummaryEditTextPreference.java
  • LearnMorePreference.java
  • WPPrefUtils.java

To test:

  • Prerequisite: You must have a Jetpack connected site, which displays this setting.
  • While on the My Site/MENU tab.
  • Click on the Jetpack Settings button.
  • Verify that the Security setting screen is displayed.
  • Click on each of the settings within the Security settings screen and verify that every setting works as expected.
5. NotificationSettingsFragment.java

Res: notifications_settings.xml

Uses:

  • NotificationsSettingsDialogPreference.java
  • WPActivityUtils.java, which uses add/removeToolbarFromDialog(...)
  • WPSwitchPreference.java, which uses SummaryEditTextPreference.java

To test:

  • While on the Notifications tab.
  • Click on the Gear setting button (top-right).
  • Verify that the Notification Settings screen is displayed.
  • Click on each of the settings within the Notification Settings settings screen and verify that every setting works as expected.

Also, consider the below usages of android.preference in non-setting screen related part of WPAndroid's codebase:

1. EditPostActivity.java

Uses:

  • EditTextPreferenceWithValidation.java, which extends from SummaryEditTextPreference.java

To test:

  • Go to Post screen.
  • Edit a new post, which uses PreferenceManager to setDefaultValues(...) for Account Settings, add a few of the main blocks and verify that everything is workings as expected.
2. MySitesPage.java (UI Test)

To test:

  • Run the StatsTests UI test suite, which uses the MySitesPage.java class, and verify that all tests pass.

From the above you will notice the below:

  1. There are 5 settings screens overall:
    • Account Settings, which corresponds to AccountSettingsFragment.kt and account_settings.xml
    • App Settings, which corresponds to AppSettingsFragment.java and app_settings.xml
    • Site Settings, which corresponds to SiteSettingsFragment.java and site_settings.xml
    • Jetpack Security Settings, which corresponds to JetpackSecuritySettingsFragment.java and jetpack_settings.xml
    • Notifications Settings, which corresponds to NotificationsSettingsFragment.java and notifications_settings.xml
  2. There are 8 custom preference classes:
    • WPPreference.java, which uses DetailListPreference.java`
    • WPStartOverPreference.java, which extends from WPPreference.java and only used by SiteSettingsFragment.java
    • WPSwitchPreference, which uses SummaryEditTextPreference.java
    • SummaryEditTextPreference
    • EditTextPreferenceWithValidation, which extends from SummaryEditTextPreference
    • DetailListPreference
    • LearnMorePreference
    • NotificationsSettingsDialogPreference, which is only used by NotificationSettingsFragment.java
  3. There are 3 extra classes related to preferences, utils or otherwise:
    • WPPrefUtils, which uses add/removeToolbarFromDialog(...)
    • WPActivityUtils
    • PreferenceFragmentLifeCycleOwner, which is extended only by AccountSettingsFragment.kt

Recommendation & Suggested Strategy

From the technical details above you will notice how interconnected everything is.

As such, my recommendation is instead of migrating all 5 settings screens, and thus, forcing us to do that change in one go, most probably even in one PR (as I am not seeing another alternative), we could create a project which will rewrite each of those screens instead. This will make this change more manageable, enable the creation of multiple PRs, each tested in isolation, and which, will target trunk directly, that is, without the need for a parent intermediate feature branch.

Per the above, my suggested strategy would be:

  • To create 5 individual PRs, per the 5 settings screens above.
  • For each PR, create a completely new screen with androidx.preference, along with creating any custom preference class that this screen might required to become fully operational.

In more detail, I would have followed the below PR chain strategy:

  • First, start with a new Account Settings screen, along with creating 3 new custom preference classes (SummaryEditTextPreference.kt, EditTextPreferenceWithValidation.kt and DetailListPreference.kt).
  • Then, continue with a new Jetpack Security Settings screen, along with creating 2 new custom preference classes (WPSwitchPreference.kt and LearnMorePreference.kt).
  • Then, continue with a new Notifications Settings screen, along with creating 1 new custom preference class (NotificationsSettingsDialogPreference.kt).
  • Then, continue with a new App Settings screen, along with creating 1 new custom preference class (WPPreference.kt).
  • Then, end with a new Site Settings screen, along with creating 1 new custom preference class (WPStartOverPreference).
  • Finally, if not already done by all the above, remove any remaining usage of android.preference from anywhere else (ie. PreferenceFragmentLifeCycleOwner, WPPrefUtils, WPActivityUtils, EditPostActivity, MySitesPage, etc)

Let's discuss, let me know your thoughts on all the above! 🙇

@peril-wordpress-mobile
Copy link

Fails
🚫

Please add a type label to this issue. e.g. '[Type] Enhancement'

Generated by 🚫 dangerJS

@irfano
Copy link
Member

irfano commented Feb 16, 2023

"Thank you for the great analysis, @ParaskP7! 🤍
I would like to expand on the discussion and ask, "Have we considered migrating to DataStore"?
I did some quick research and found a comparison between SharedPreferences and DataStore. Although there are benefits to using DataStore, I haven't checked its specific advantages for our project. Instead of calling preferences in views, we could create a UserDataRepository and call its functions in ViewModels.

I also tried to find advantages of androidx.preferences over DataStore. To my understanding, using androidx.preferences allows us to easily populate PreferenceFragment with SwitchPreferences without handling the data part. Probably, we can do the same with DataStore. 🤔

In summary, DataStore provides ACID guarantees, but androidx.preferences seems more practical.
What do you think about migrating to DataStore?

@ParaskP7
Copy link
Contributor Author

👋 @irfano !

That's a great question and thanks for raising it! 💯

I too wanted to suggest an extra step to all the above, which would be to move away from our current SharedPreferences (Save key-value data) solution and go for a DataStore specific solution. But, then, I stepped back and started thinking, SharedPreferences and Preference is two district things, of course they are! 😅

The above means that Preference is not going away and can be used in combination with DataStore, just like it is currently used in combination with SharedPreferences.

This to me means that we shouldn't confused the 2 migrations, migrating from one UI layer solution (android.preference) to another (androidx.preference), and, at the same time, migrating from on Data layer solution (SharedPreferences) to another (DataStore). It is just unfortunate that Preference (UI) and SharedPreferences (Data) are sharing the preference naming, that's what makes this confusing, isn't it? 😅

Let me know if I am missing something obvious here, I for sure might... 👍

@irfano
Copy link
Member

irfano commented Feb 17, 2023

So, this issue is not related to SharedPreferences. 😅 It does not affect this issue and should be submitted as a completely separate PR.
Thank you for clarifying my confusion with your explanation.

@zwarm
Copy link
Contributor

zwarm commented Mar 9, 2023

@ParaskP7 - While reviewing #17780 I stumbled upon this ticket. Kahu is in Release team rotation, so I may have time to start digging into the following. I will post here if was able to make any progress. Would be nice if we could use compose too :P

First, start with a new Account Settings screen, along with creating 3 new custom preference classes (SummaryEditTextPreference.kt, EditTextPreferenceWithValidation.kt and DetailListPreference.kt).

@ParaskP7
Copy link
Contributor Author

👋 @zwarm !

While reviewing #17780 I stumbled upon this ticket. Kahu is in Release team rotation, so I may have time to start digging into the following.

Coolio and thank you! 💪

PS: Just don't underestimate this task, it seems (kind of) straightforward but it is not, be prepared for unknowns/blockers/interdependencies... 😅

I will post here if was able to make any progress.

Awesome and best of luck! 🍀

Would be nice if we could use compose too :P

💯 😝

@irfano
Copy link
Member

irfano commented Apr 17, 2023

We should also remove tools:ignore="FragmentTagUsage" usages, and replace <fragment> with FragmentContainerView in PreferenceFragments after the migration. See e050b18

@mkevins
Copy link
Contributor

mkevins commented May 30, 2023

Just chiming in to say that I observed the theme switch issue (invisible text like in this issue: #17780) in the Account Settings when developing the Account Closure feature.

Would be nice if we could use compose too :P

Indeed, Compose would be great! Fyi, I've already planted a tiny "seed" of Compose in the Account Settings screen, just for the "Close Account" button, and was surprised how easy it was to integrate it with the existing UI. I was thinking it would be good to have this whole screen in Compose, so I was glad to find this idea already mentioned. 🎉

@07jasjeet
Copy link
Contributor

Hello @ParaskP7! I did the migration of NotificationsSettingsFragment (PreferenceFragment) to AndroidX PreferenceFragmentCompat due to blockers caused by PreferenceFragment in my originally intended work.

Sidenote: PreferenceFragmentCompat supports DataStore as well.

@ParaskP7
Copy link
Contributor Author

👋 @07jasjeet and apologies for the late reply, I was on parental leave and just returned back to action, still catching-up on everything. Having said that, I am seeing @antonis picked this up and already helped you by reviewing this #19986 PR of yours, many thanks Antonis! 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants