Preference Migration Part-1: Migrated NotificationsSettingsDialogPreference #19986
Conversation
Created AndroidX implementation of NotificationsSettingsDialogPreference.java
Generated by 🚫 dangerJS |
| import org.wordpress.android.util.JSONUtils | ||
| import org.wordpress.android.util.extensions.getDrawableFromAttribute | ||
|
|
||
| class NotificationsSettingsDialogFragment( |
There was a problem hiding this comment.
I would advocate toward using fragment arguments instead of custom constructor parameters. I found this article useful on how to do this. This example of a DialogFragment using this pattern inside the project can be useful.
| NotificationsSettings.Channel.OTHER, NotificationsSettings.Channel.WPCOM -> {} | ||
| } | ||
| if (mTitleViewWithMainSwitch != null) { | ||
| val titleView = mTitleViewWithMainSwitch!!.findViewById<TextView>(R.id.title) |
There was a problem hiding this comment.
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.
Since this specific case involves a View we could use a lateinit variable instead of a nullable one and not have the need for a !!. This can be applied in all interface objects used. Wdyt?
When using a nullable object is not avoidable it is preferred to use the safe call operator ?. or a simple null check.
You can find some more information on null safety in Kotlin here.
|
|
||
| // Update the settings json | ||
| val keys: Iterator<*> = mUpdatedJson.keys() | ||
| while (keys.hasNext()) { |
There was a problem hiding this comment.
What would you say of simplifying the while block with something like the following:
for (settingName in mUpdatedJson.keys()) {
settings.updateSettingForChannelAndType(
this.channel, this.type, settingName,
mUpdatedJson.optBoolean(settingName), this.blogId
)
}
|
|
||
| override fun onCreateDialog(savedInstanceState: Bundle?): Dialog { | ||
| val context = requireContext() | ||
| @SuppressLint("InflateParams") |
There was a problem hiding this comment.
We could avoid the use the suppress by using the onCreateView method. This is an example DialogFragment that does this.
|
Thank you for your contribution @07jasjeet 🙇 I've left some feedback inline your code.
Could you help me with this to be able to test the UI properly (e.g. a patch file)? I also noticed a lint error (you can trigger it with
We could annotate the |
Created AndroidX implementation of
NotificationsSettingsDialogPreference.javaDid not remove
NotificationsSettingsDialogPreference.javadue to compatibility issues but will be removed onceNotificationsSettingsFragmenthas been migrated.Fixes #17962 (partially)
Review Notes: Compare with
NotificationsSettingsDialogPreference.javaandNotificationsSettingsFragmentto draw conclusions.To Test:
Testing can be done in upcoming PRs. Otherwise, supplying dummy data and showing this dialog on any screen would work as well.
Regression Notes
Potential unintended areas of impact
None
What I did to test those areas of impact (or what existing automated tests I relied on)
Manual Testing
What automated tests I added (or what prevented me from doing so)
None
PR Submission Checklist:
RELEASE-NOTES.txtif necessary.UI Changes Testing Checklist: