Part-4: Compiler Warnings as Errors - WordPress Module - FragmentManager and PreferenceFragment

[07jasjeet]

Partially fixes #17253 and #17962

This PR include complete migration of NotificationsSettingsFragment to AndroidX implementation and migration of deprecated FragmentManager usages. All the links and answers to some expected questions have been given in commits description itself.

A few mistakes to be considered while reading commit https://github.com/wordpress-mobile/WordPress-Android/pull/19876/commits/275795ffbe3fd9f40c4203b2007b12cc6b072388 's description:

1. Point 3 is duplicated from 2 but the actual message was:

   Removed My Sites child preference screen in favour of recommended implementation.

2. The link mentioned is wrong, this is the intended link.

To Test:

1. Navigate to Notifications screen.
2. On top right corner, click on the settings logo.
3. Test EVERYTHING in this newly opened fragment

Regression Notes

1. Potential unintended areas of impact

   Notifications Settings screen's child screens

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

   Manual testing

3. What automated tests I added (or what prevented me from doing so)

   None

---

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.

---

UI Changes Testing Checklist:

• [ ] Portrait and landscape orientations.
• [ ] Light and dark modes.
• [ ] Fonts: Larger, smaller and bold text.
• [ ] High contrast.
• [ ] Talkback.
• [ ] Languages with large words or with letters/accents not frequently used in English.
• [ ] Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
• [ ] Large and small screen sizes. (Tablet and smaller phones)
• [ ] Multi-tasking: Split screen and Pop-up view. (Android 10 or higher)

[07jasjeet]

@antonis Pr is ready for review!

[07jasjeet]

Note: Component left in PR for easy review: NotificationsSettingsDialogPreference which should be removed before this PR is merged.

[peril-wordpress-mobile[bot]]

	Warnings
:warning:	PR is not assigned to a milestone.
:warning:	PR has more than 300 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by :no_entry_sign: dangerJS

[antonis]

@antonis Pr is ready for review!

Thank you for your work on this @07jasjeet 🙇 My plan is to review this in detail next week.

I was wondering if the commits can be split into smaller PRs that can be reviewed and tested independently? If there are dependencies the PRs can be chained and merged one by one. We rarely land so big code changes in one batch. Eg. could we split the refactoring or the Android X work https://github.com/wordpress-mobile/WordPress-Android/issues/17962#issuecomment-1886494816 into separate PRs. The recommended size is <300 and anything above that triggers CI warning. On some occasions we may go above that but usually this involves test code.

Note: Component left in PR for easy review: NotificationsSettingsDialogPreference which should be removed before this PR is merged.

This can also be included if we extract the related work in a separate PR.

[07jasjeet]

I was wondering if the commits can be split into smaller PRs that can be reviewed and tested independently? If there are dependencies the PRs can be chained and merged one by one. We rarely land so big code changes in one batch. Eg. could we split the refactoring or the Android X work #17962 (comment) into separate PRs. The recommended size is <300 and anything above that triggers CI warning. On some occasions we may go above that but usually this involves test code.

Note: Component left in PR for easy review: NotificationsSettingsDialogPreference which should be removed before this PR is merged.

This can also be included if we extract the related work in a separate PR.

Alright, so each commit will have its own PRs. Considering that, we need some PRs as follows:

1. Migration of NotificationsSettingsDialogPreference. https://github.com/wordpress-mobile/WordPress-Android/pull/19876/commits/c7cbb31524da9e3c9f07ac7264d3e442af7fde03
2. With the branch of PR 1 as base, Creation of AndroidX implementation of WPSwitchPreference. https://github.com/wordpress-mobile/WordPress-Android/pull/19876/commits/9372604a8bc040dc63dc6a4e72c3cfa7d0439687
3. With the branch of PR 2 as base, Creation of Notification Types Fragment with Child fragment because both depend on each other. https://github.com/wordpress-mobile/WordPress-Android/pull/19876/commits/d2d3cb1366651d35ab8e0aba5416f8054bd78937
4. With the branch of PR 3 as base, Creation of My Sites Settings fragment. https://github.com/wordpress-mobile/WordPress-Android/pull/19876/commits/a61466b30d9b11e6dd010a976ac30b4417413d32
5. With the branch of PR 4 as base, This commit cannot be split as everything in this commit is dependent on the previously mentioned PRs, https://github.com/wordpress-mobile/WordPress-Android/pull/19876/commits/275795ffbe3fd9f40c4203b2007b12cc6b072388 In this 5th PR alone, we would have https://github.com/wordpress-mobile/WordPress-Android/pull/19876/commits/7bd1a5f84944805dcbc1f43ad610c6191b38321c and https://github.com/wordpress-mobile/WordPress-Android/pull/19876/commits/d9e40e2ebc0d2de28fae8787b1d721b2d49cdba4 as the code is intertwined with each other so much that things would break separately.

[antonis]

Alright, so each commit will have its own PRs.

The above PR split looks good to me 👍

[antonis]

Hey @07jasjeet 👋 As discussed on Slack I'll dive deeper on the PR changes and provide feedback on this PR next week before deciding on if/how to split your contribution in more PRs.

I've run some automated test and found the following.

Checkstyle

[ant:checkstyle] [ERROR] WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsMySitesFragment.kt:85: Specific imports should be used for FluxC Store inner classes [RegexpSingleline]
[ant:checkstyle] [ERROR] WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsMySitesFragment.kt:95: Specific imports should be used for FluxC Store inner classes [RegexpSingleline]
[ant:checkstyle] [ERROR] WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsMySitesSettingsFragment.kt:32: Specific imports should be used for FluxC Store inner classes [RegexpSingleline]
[ant:checkstyle] [ERROR] WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsMySitesSettingsFragment.kt:48: Specific imports should be used for FluxC Store inner classes [RegexpSingleline]
[ant:checkstyle] [ERROR] WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsMySitesSettingsFragment.kt:89: Specific imports should be used for FluxC Store inner classes [RegexpSingleline]
[ant:checkstyle] [ERROR] WordPress/src/main/java/org/wordpress/android/models/NotificationsSettings.java:3:8: Unused import - androidx.annotation.NonNull. [UnusedImports]

Detekt

WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsDialogFragment.kt:260:39: The function setupSwitchSettingView(settingName: String, settingValue: String?, settingSummary: String?, isSettingChecked: Boolean, isSettingLast: Boolean, onCheckedChangeListener: CompoundButton.OnCheckedChangeListener) has too many parameters. The current threshold is set to 6. [LongParameterList]
WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsDialogFragment.kt:282:33: The function setupSettingView(settingName: String, settingValue: String?, settingSummary: String?, isSettingChecked: Boolean, isSettingLast: Boolean, onCheckedChangeListener: CompoundButton.OnCheckedChangeListener?, onClickListener: View.OnClickListener?) has too many parameters. The current threshold is set to 6. [LongParameterList]
WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsDialogFragment.kt:59:18: The function onCreateDialog is too long (61). The maximum length is 60. [LongMethod]
WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsDialogFragment.kt:174:17: The function configureLayoutForView appears to be too complex based on Cyclomatic Complexity (complexity: 16). Defined complexity threshold for methods is set to '15' [CyclomaticComplexMethod]
WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsMySitesFragment.kt:111:17: The function configureFollowedBlogsSettings is too long (63). The maximum length is 60. [LongMethod]
WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsMySitesSettingsFragment.kt:22:9: The function onMySiteSettingsChanged is too long (77). The maximum length is 60. [LongMethod]
WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsDialogPreferenceX.kt:8:45: The constructor(context: Context, attrs: AttributeSet?, channel: NotificationsSettings.Channel, type: NotificationsSettings.Type, blogId: Long, settings: NotificationsSettings, listener: NotificationsSettingsDialogPreference.OnNotificationsSettingsChangedListener, bloggingRemindersProvider: NotificationsSettingsDialogPreference.BloggingRemindersProvider?, dialogTitleRes: Int) has too many parameters. The current threshold is set to 7. [LongParameterList]
WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsFragment.kt:587:17: The function configureFollowedBlogsSettings is too long (85). The maximum length is 60. [LongMethod]
WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsFragment.kt:75:7: Class NotificationsSettingsFragment is too large. Consider splitting it into smaller pieces. [LargeClass]
WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsFragment.kt:527:17: The function configureBlogsSettings appears to be too complex based on Cyclomatic Complexity (complexity: 16). Defined complexity threshold for methods is set to '15' [CyclomaticComplexMethod]
WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsFragment.kt:587:17: The function configureFollowedBlogsSettings appears to be too complex based on Cyclomatic Complexity (complexity: 18). Defined complexity threshold for methods is set to '15' [CyclomaticComplexMethod]
WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsFragment.kt:132:85: This empty block of code can be removed. [EmptyFunctionBlock]
WordPress/src/main/java/org/wordpress/android/ui/prefs/WPSwitchPreferenceX.kt:83:17: Function getSwitch has 3 return statements which exceeds the limit of 2. [ReturnCount]
WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsTypesFragment.kt:153:1: Line detected, which is longer than the defined maximum line length in the code style. [MaxLineLength]
WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/ChildNotificationSettingsFragment.kt:30:35: This expression contains a magic number. Consider defining it to a well named constant. [MagicNumber]
WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsDialogFragment.kt:35:1: Line detected, which is longer than the defined maximum line length in the code style. [MaxLineLength]
WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsMySitesFragment.kt:127:14: The destructuring declaration contains 4 but only 3 are allowed. [DestructuringDeclarationWithTooManyEntries]
WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsMySitesSettingsFragment.kt:107:1: Line detected, which is longer than the defined maximum line length in the code style. [MaxLineLength]
WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsMySitesSettingsFragment.kt:110:1: Line detected, which is longer than the defined maximum line length in the code style. [MaxLineLength]
WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsDialogPreferenceX.kt:7:58: There should be exactly one empty line in between the list of imports and the declaration of NotificationsSettingsDialogPreferenceX. [SpacingBetweenPackageAndImports]
WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsFragment.kt:611:14: The destructuring declaration contains 4 but only 3 are allowed. [DestructuringDeclarationWithTooManyEntries]
WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsFragment.kt:505:1: Line detected, which is longer than the defined maximum line length in the code style. [MaxLineLength]
WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsFragment.kt:72:27: There should be exactly one empty line in between the list of imports and the declaration of NotificationsSettingsFragment. [SpacingBetweenPackageAndImports]

[07jasjeet]

Checkstyle

[ant:checkstyle] [ERROR] WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsMySitesFragment.kt:85: Specific imports should be used for FluxC Store inner classes [RegexpSingleline]
[ant:checkstyle] [ERROR] WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsMySitesFragment.kt:95: Specific imports should be used for FluxC Store inner classes [RegexpSingleline]
[ant:checkstyle] [ERROR] WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsMySitesSettingsFragment.kt:32: Specific imports should be used for FluxC Store inner classes [RegexpSingleline]
[ant:checkstyle] [ERROR] WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsMySitesSettingsFragment.kt:48: Specific imports should be used for FluxC Store inner classes [RegexpSingleline]
[ant:checkstyle] [ERROR] WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsMySitesSettingsFragment.kt:89: Specific imports should be used for FluxC Store inner classes [RegexpSingleline]
[ant:checkstyle] [ERROR] WordPress/src/main/java/org/wordpress/android/models/NotificationsSettings.java:3:8: Unused import - androidx.annotation.NonNull. [UnusedImports]

Not sure on how to solve the Specific imports should be used for FluxC Store inner classes check style issue. This was being used before in older implementations without any warnings but that is not the case here. Can you educate me about what this warning actually means?

[antonis]

Not sure on how to solve the Specific imports should be used for FluxC Store inner classes check style issue. This was being used before in older implementations without any warnings but that is not the case here.

Good question @07jasjeet 👍

The specific rule is related to how we import WordPress-FluxC-Android inner classes. It can be handled by using the full import of the inner class (e.g. in NotificationsSettingsFragment.java)

I've prepared a patch for the specific cases below. RegexpSingleline.patch

This rule may seem opinionated but it is part of the Android Code Style Guidelines for Contributors that we are using.

[antonis]

Hey @07jasjeet 👋 thank you for the submitted fixes. All the checkstyle issues have been resolved and most of the Detekt issue. The following Detekt issues persist. Let me know if I can help with those 🙇

WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsDialogFragment.kt:253:39: The function setupSwitchSettingView(settingName: String, settingValue: String?, settingSummary: String?, isSettingChecked: Boolean, isSettingLast: Boolean, onCheckedChangeListener: CompoundButton.OnCheckedChangeListener) has too many parameters. The current threshold is set to 6. [LongParameterList]
WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsDialogFragment.kt:272:33: The function setupSettingView(settingName: String, settingValue: String?, settingSummary: String?, isSettingChecked: Boolean, isSettingLast: Boolean, onCheckedChangeListener: CompoundButton.OnCheckedChangeListener?, onClickListener: View.OnClickListener?) has too many parameters. The current threshold is set to 6. [LongParameterList]
WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsDialogFragment.kt:170:17: The function configureLayoutForView appears to be too complex based on Cyclomatic Complexity (complexity: 16). Defined complexity threshold for methods is set to '15' [CyclomaticComplexMethod]
WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsDialogPreferenceX.kt:9:45: The constructor(context: Context, attrs: AttributeSet?, channel: NotificationsSettings.Channel, type: NotificationsSettings.Type, blogId: Long, settings: NotificationsSettings, listener: NotificationsSettingsDialogPreference.OnNotificationsSettingsChangedListener, bloggingRemindersProvider: NotificationsSettingsDialogPreference.BloggingRemindersProvider?, dialogTitleRes: Int) has too many parameters. The current threshold is set to 7. [LongParameterList]
WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsFragment.kt:74:7: Class NotificationsSettingsFragment is too large. Consider splitting it into smaller pieces. [LargeClass]
WordPress/src/main/java/org/wordpress/android/ui/prefs/WPSwitchPreferenceX.kt:83:17: Function getSwitch has 3 return statements which exceeds the limit of 2. [ReturnCount]
WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsMySitesFragment.kt:134:14: The destructuring declaration contains 4 but only 3 are allowed. [DestructuringDeclarationWithTooManyEntries]
WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsFragment.kt:632:14: The destructuring declaration contains 4 but only 3 are allowed. [DestructuringDeclarationWithTooManyEntries]

I will iterate with more feedback on the PR.

[07jasjeet]

Hey @07jasjeet 👋

thank you for the submitted fixes. All the checkstyle issues have been resolved and most of the Detekt issue.

The following Detekt issues persist. Let me know if I can help with those 🙇

WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsDialogFragment.kt:253:39: The function setupSwitchSettingView(settingName: String, settingValue: String?, settingSummary: String?, isSettingChecked: Boolean, isSettingLast: Boolean, onCheckedChangeListener: CompoundButton.OnCheckedChangeListener) has too many parameters. The current threshold is set to 6. [LongParameterList]

WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsDialogFragment.kt:272:33: The function setupSettingView(settingName: String, settingValue: String?, settingSummary: String?, isSettingChecked: Boolean, isSettingLast: Boolean, onCheckedChangeListener: CompoundButton.OnCheckedChangeListener?, onClickListener: View.OnClickListener?) has too many parameters. The current threshold is set to 6. [LongParameterList]

WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsDialogFragment.kt:170:17: The function configureLayoutForView appears to be too complex based on Cyclomatic Complexity (complexity: 16). Defined complexity threshold for methods is set to '15' [CyclomaticComplexMethod]

WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsDialogPreferenceX.kt:9:45: The constructor(context: Context, attrs: AttributeSet?, channel: NotificationsSettings.Channel, type: NotificationsSettings.Type, blogId: Long, settings: NotificationsSettings, listener: NotificationsSettingsDialogPreference.OnNotificationsSettingsChangedListener, bloggingRemindersProvider: NotificationsSettingsDialogPreference.BloggingRemindersProvider?, dialogTitleRes: Int) has too many parameters. The current threshold is set to 7. [LongParameterList]

WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsFragment.kt:74:7: Class NotificationsSettingsFragment is too large. Consider splitting it into smaller pieces. [LargeClass]

WordPress/src/main/java/org/wordpress/android/ui/prefs/WPSwitchPreferenceX.kt:83:17: Function getSwitch has 3 return statements which exceeds the limit of 2. [ReturnCount]

WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsMySitesFragment.kt:134:14: The destructuring declaration contains 4 but only 3 are allowed. [DestructuringDeclarationWithTooManyEntries]

WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsFragment.kt:632:14: The destructuring declaration contains 4 but only 3 are allowed. [DestructuringDeclarationWithTooManyEntries]

I will iterate with more feedback on the PR.

Hello @antonis These detekt issues were present before my PR as well, as stated in the last commit's description.

These are unavoidable in my eye. I do need some suggestions as to how we should move forward with this.

[antonis]

Thank you for your quick response @07jasjeet and your contribution 🙇

These detekt issues were present before my PR as well, as stated in the last commit's description.

My understanding is that those are related to new code introduced in this PR. Probably the checks on the equivalent java code were different or suppressed.

These are unavoidable in my eye. I do need some suggestions as to how we should move forward with this.

For the two LongParameterList in the NotificationsSettingsDialogFragment we could create a data class to hold the settings related values. E.g.

    data class NotificationSetting(
        val settingName: String,
        val settingValue: String?,
        val settingSummary: String?,
        val isSettingChecked: Boolean,
        val isSettingLast: Boolean
    )

We can even add some default values and initialise this only with the properties needed on each case.

For the CyclomaticComplexMethod in NotificationsSettingsDialogFragment I would suggest splitting the method into more than one.

For the LongParameterList in NotificationsSettingsDialogPreferenceX we could create a data class like above.

The NotificationsSettingsFragment (LargeClass error) replaced an even larger Java class. Are there opportunities to split some of the functionality in a different class (e.g. a utility class)?

Regarding the DestructuringDeclarationWithTooManyEntries in NotificationsSettingsMySitesFragment and NotificationsSettingsFragment we could use the models directly. E.g.

for (model in models) {
//..
prefScreen.title = model.title
//...

or

models.forEach { 
 //...
prefScreen.title = it.title
 //...
}

The ReturnCount on the recursive getSwitch function of WPSwitchPreferenceX can either be suppressed or come up with a better way to implement this in Kotlin.

Trying to compile the code I also get the following warnings which are treated as errors in our configuration.

w: WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsDialogFragment.kt:234:13 Parameter 'compoundButton' is never used, could be renamed to _
w: WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsDialogFragment.kt:247:13 Parameter 'v' is never used, could be renamed to _
w: WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsDialogFragment.kt:287:51 Parameter 'v' is never used, could be renamed to _
w: WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsFragment.kt:121:9 'setHasOptionsMenu(Boolean): Unit' is deprecated. Deprecated in Java

One other generic issue not caught by the tools is the use of not-null assertion operator (!!) in many places in the introduced code. Though this may be introduced when converting code to Kotlin we should try to avoid it.

Taking a step back I think that we should return in our plan to split the code into multiple PRs that can be reviewed and tested individually. Do we need all the changes for https://github.com/wordpress-mobile/WordPress-Android/issues/17253 or we enlarged the scope to also cover https://github.com/wordpress-mobile/WordPress-Android/issues/17962?

[07jasjeet]

Taking a step back I think that we should return in our plan to split the code into multiple PRs that can be reviewed and tested individually. Do we need all the changes for #17253 or we enlarged the scope to also cover #17962?\

Alright Ill try to split commits as much as I can.

As for the rest, I'll not make commits to this PR anymore and rather deal with these detekt issues in separate the PRs.

[irfano]

Hey @07jasjeet! Would you still like to work on this? Should we keep this PR open?

[jkmassel]

Closing due to inactivity, but we would be happy to review further contributions in the future!