Nullability Annotations to Java Classes - DialogFragment - Replace findViewById with ViewBinding

[ParaskP7]

Parent #18911

This issue is about adding replacing findViewById(...) with ViewBinding to as many Java-related DialogFragment classes as possible.

Instead of adding missing nullability annotations (@Nullable & @NonNull) to layout View related fields on such Java-related DialogFragment classes, it is better to migrate those fields, from the old way of assigning those (using findViewById(...)), and into the new way of referencing such view (direct via ViewBinding.

FYI: You could reference #14845 to get an idea on how to go about that.

---

Tasks (libs/editor)

Beta Give feedback

1. ImageSettingsDialogFragment.java
2. LinkDialogFragment.java

Tasks (WordPress + ui.people)

Beta Give feedback

1. RoleChangeDialogFragment.java
2. RoleSelectDialogFragment.java

Tasks (WordPress + ui.posts)

Beta Give feedback

1. AddCategoryFragment.java
2. InsertMediaDialog.java
3. PostSettingsInputDialogFragment.java
4. PostSettingsListDialogFragment.java

Tasks (WordPress + ui.publicize)

Beta Give feedback

1. PublicizeAccountChooserDialogFragment.java

Tasks (WordPress + ui)

Beta Give feedback

1. CollapseFullScreenDialogFragment.java
2. FullScreenDialogFragment.java
3. TextInputDialogFragment.java
4. WPBottomSheetDialogFragment.java

Tasks (WordPress + widgets)

Beta Give feedback

1. AuthErrorDialogFragment.java

Tasks (WordPress + inner static)

Beta Give feedback

1. PluginDetailActivity.java -> DomainRegistrationPromptDialog
2. WPPrefView.java -> WPPrefDialogFragment

[peril-wordpress-mobile]

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

Generated by 🚫 dangerJS

[peril-wordpress-mobile]

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

Generated by 🚫 dangerJS

[peril-wordpress-mobile]

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

Generated by 🚫 dangerJS

[Areeb786123]

can you please assign me this issue?

[ParaskP7]

👋 @Areeb786123 and thank you so much for offering to contribute! 🥇

Please feel free to start working on this. As such, I'll go ahead and assign this task to you (and me). PS: I am assigning myself as well, as I'll be guiding you all the way, up until we collaboratively merge your solution to trunk.

Before you begin, make sure to at least read the following documentation:

• README.md
  • Build Instructions (Step.2): You might notice that you would need to install npm. This is done for block editor related purposes (see gutenberg and gutenberg-mobile). The block editor is all React Native and as such this is a requirement for when working on it, especially for testing purposes. However, feel free to skip this step, that is, until you need it. PS: Most probably you won't be needing it for this task.
• CONTRIBUTING.md
• docs/coding-style.md

FYI: Note that depending on your solution, number of PRs, PR description, commit strategy, etc, I might ask you to improve on the quality and sometimes, even redo a PR. But, don't be afraid of it, I will only do that to help you out. Since this will be one of your first contribution to this repo, it is all part of the learning process, that is, until you become comfortable and aware about the ins-and-outs of how we work.

Do let me know if you need anything else! 🌟

PS: You can also check #18906 to get some inspiration on how to go about when working on your commits and then finally creating your PRs. I hope that helps. 🙏

[Areeb786123]

@ParaskP7 thanks for assigning me i will complete it as soon as possible

[ParaskP7]

👋 @Areeb786123 and thank you! 💯

...i will complete it as soon as possible

There is no rush, please take you time. Actually, it is better if you do it right, rather than fast.

FYI: Please create a new PR per task above (aka per class), this way it will be more manageable for us to review, test and finally merge any of your PRs.

[Areeb786123]

@ParaskP7 OK sir thanks for the guidance.

[neeldoshii]

@ParaskP7 Can I work on this issue?

[ParaskP7]

👋 @neeldoshii and thank you so much for offering to contribute! 🥇

Please feel free to start working on this. As such, I'll go ahead and re-assign this task to you now.

FYI: As per the above message, please make sure to read the documentation first. Also, I would suggest for you to create a new PR per task above (aka per class), this way it will be more manageable for us to review, test and finally merge any of your PRs.

[neeldoshii]

Thank you Petros
Did a quite digging, we no longer have ImageSettingDialogFragment,PostSettingsListDialogFragment.java doesn't have findViewById so we can remove it from TODO.

Can you provide steps to reproduce the following fragment in the app? I did migration to ViewBinding locally but I am unsure about the impact wanna test it first before pushing.
1. PostSettingsInputDialogFragment.java [Edit Reproduced, thanks to @twstokes for providing git bisect.]
2. InsertMediaDialog.java

Also as a commit strategy as usual, would it be okay for one class migration as a commit for ease in reverting if needed?

My current plan is to migrate Tasks (WordPress + ui.posts) first. Just a followup improvement once the migration to viewbinding is complete can we migrate it to kotlin as it will be very useful for later migrating to compose. As most of the Automaticc apps are slowly migrating to compose.

[ParaskP7]

👋 @neeldoshii and thanks for starting work on that! 🌟

Did a quite digging, we no longer have ImageSettingDialogFragment,PostSettingsListDialogFragment.java doesn't have findViewById so we can remove it from TODO.

You are correct, I just marked them as done, just to get them out of the way. ✅

Can you provide steps to reproduce the following fragment in the app? I did migration to ViewBinding locally but I am unsure about the impact wanna test it first before pushing.

We just want to make sure that the change is not breaking anything and thus impacting the app negatively. Thus, we should make sure that we can test it first, before the change, and then again, after the change, and this way to assert correctness of behavior.

1. PostSettingsInputDialogFragment.java [Edit Reproduced, thanks to @twstokes for providing git bisect.]

👍

2. InsertMediaDialog.java

Actually, I would need to invest some time into reproducing this dialog myself. But, this is actually part of the task itself, to first try and figure out how to reproduce a screen/view/dialog. Sometimes, this might even be a dead-code that we could just need to "safely" remove instead.

Also as a commit strategy as usual, would it be okay for one class migration as a commit for ease in reverting if needed?

First, I would suggest one class per PR, that is, unless a group of classes are related to a single specific screen and/or functionality.

Then, as far as the commit strategy is concerned, I suggest multiple commits if possible, but self-sufficient, commits that do everything that they describe, but also don't do too many things at once. For example, if you need to refactor something, that is, before replacing a findViewById, a group of findViewById or all of them in one go, I would first suggest committing this unrelated refactor work first.

Doest that help? 😊

My current plan is to migrate Tasks (WordPress + ui.posts) first. Just a followup improvement once the migration to viewbinding is complete can we migrate it to kotlin as it will be very useful for later migrating to compose. As most of the Automaticc apps are slowly migrating to compose.

As long as we take this PR by PR, one step at a time, your plan sounds good to me! 💯

Once more thanks for all your current and future contributions Neel! 🥇

[neeldoshii]

Hi @ParaskP7,

You are correct, I just marked them as done, just to get them out of the way. ✅

👍

First, I would suggest one class per PR, that is, unless a group of classes are related to a single specific screen and/or functionality.

Then, as far as the commit strategy is concerned, I suggest multiple commits if possible, but self-sufficient, commits that do everything that they describe, but also don't do too many things at once. For example, if you need to refactor something, that is, before replacing a findViewById, a group of findViewById or all of them in one go, I would first suggest committing this unrelated refactor work first.

Gotcha, Rebasing #20933 and splitting it into two PR as this has 2 classes covered AddCategoryFragment & PostSettingsInputDialogFragment.

One more thing to ask.

AddCategoryFragment

Would it be okay if I migrate this class to kotlin due to two reason 1. We still have 8 null annotation lint warning even after migrating to viewbinding 2. Sooner or later we eventually will need to migrate it.

PostSettingsInputDialogFragment

Same with PostSettingsInputDialogFragment.

Current plan : As a first step I am migrating currently to viewbinding in two PR then once this get merged without breaking then convert to kotlin (if approved).

[ParaskP7]

👋 @neeldoshii !

Gotcha, Rebasing #20933 and splitting it into two PR as this has 2 classes covered AddCategoryFragment & PostSettingsInputDialogFragment.

👍

Would it be okay if I migrate this class (AddCategoryFragment) to kotlin due to two reason 1. We still have 8 null annotation lint warning even after migrating to viewbinding 2. Sooner or later we eventually will need to migrate it.

Same with PostSettingsInputDialogFragment.

Just to keep it simple, let's avoid the Kotlin migration and do it on separate PR(s) for now. Later on, when we gain more confidence over this process, and in us working together, we might consider doing both in a single PR. The truth is, I just don't want to overwhelmed you with (potential) extra PR comments, just because of the extra Kotlinization work (commits), risking your contributions/PR to stay still and maybe even not merge into trunk.

PS: Don't worry too much about the warnings you see there. Most of them are nullability related warnings, which are anyway a prerequisite for a class Kotlinization process.

Current plan : As a first step I am migrating currently to viewbinding in two PR...

👍

...then once this get merged without breaking then convert to kotlin (if approved).

Let's avoid this for a later (subsequent) set of PRs.

[neeldoshii]

Hi @ParaskP7 !

I have rebased and splitted it into Two PR (#20933 & #20941) and ready for review.

Did more digging for Tasks (WordPress + widgets) AuthErrorDialogFragment doesn't contains any findViewById it just an dialog without customview. We can let this done.

Edit :
I believe Tasks (WordPress + ui.people) is no longer used in our app and is dead code. I was unable to find any PR related to removal.
I found the steps to reproduce in the app (but it seems it no longer can be seen on the app).

PR #19568 says the steps to reproduce

1. Have at least one site.
2. Navigate to "My Site" tab.
3. Click on "More".
4. Click on "People".
5. On top-left corner, click on the "+" button.
6. You should have a form in-front of you that is used to invite people.
7. Click on the "Role" field.
8. A dialog should pop us as expected.

Issue #8376 (comment) & #8376 (comment) also says the steps to reproduce this but this dialog is no longer used latest version of wordpress. I am not able to reproduce this in iOS and Android version. also doesn't have any findviewbyid

[ParaskP7]

👋 @neeldoshii !

I have rebased and splitted it into Two PR (#20933 & #20941) and ready for review.

Awesome, thanks, I'll get into reviewing those next week! 🌟

Did more digging for Tasks (WordPress + widgets) AuthErrorDialogFragment doesn't contains any findViewById it just an dialog without customview. We can let this done.

Great, you're right, I just marked this as done as well! ✅

I believe Tasks (WordPress + ui.people) is no longer used in our app and is dead code. I was unable to find any PR related to removal.
I found the steps to reproduce in the app (but it seems it no longer can be seen on the app).

PR #19568 says the steps to reproduce
...

Interesting, let me take a look at it and will get back to you on that. 🤔

Issue #8376 (comment) & #8376 (comment) also says the steps to reproduce this but this dialog is no longer used latest version of wordpress. I am not able to reproduce this in iOS and Android version.

The dialogs you mentioned with the linked comments are Site Dialog related, not Role Change/Select Dialog, right? Am I missing something? 🤔

also doesn't have any findviewbyid

Do you mean the RoleChangeDialogFragment, which actually does have few findViewById? 🤔

But, yes, looking at RoleSelectDialogFragment it doesn't and I went ahead and marked at least that as done. ✅

[neeldoshii]

The dialogs you mentioned with the linked comments are Site Dialog related, not Role Change/Select Dialog, right? Am I missing something? 🤔

Apologies. I pasted two comments from the PR which created confusion.
#8376 (comment) in subpart two Jorge Casariego explained steps which I believe is RoleChangeDialogFragment as per the steps we no longer have this flow in our current version. Not sure about which dialog it is RoleChangeDialogFragment or RoleSelectDialogFragment but the reproduction step match with the OP in #19568

---

If this is no longer used its safe to say both (RoleChangeDialogFragment or RoleSelectDialogFragment) the next step would be safely removing the dialog without breaking the app.

---

also doesn't have any findviewbyid

Do you mean the RoleChangeDialogFragment, which actually does have few findViewById? 🤔

But, yes, looking at RoleSelectDialogFragment it doesn't and I went ahead and marked at least that as done. ✅

Yes RoleSelectDialogFragment

[ParaskP7]

👋 @neeldoshii and a happy new week to you! ☀️

If this is no longer used its safe to say both (RoleChangeDialogFragment or RoleSelectDialogFragment) the next step would be safely removing the dialog without breaking the app.

Yes, let me first investigate that real quick, just to verify that both RoleChangeDialogFragment or RoleSelectDialogFragment are not used and then I'll maybe ping you for a removal PR, wdyt? 😊

[ParaskP7]

Yes, let me first investigate that real quick, just to verify that both RoleChangeDialogFragment or RoleSelectDialogFragment are not used and then I'll maybe ping you for a removal PR, wdyt? 😊

After investigating those real quick, I found out that both the RoleChangeDialogFragment and RoleSelectDialogFragment are indeed being used in the app(s). Thus we cannot (safely) remove them. Again, as per my comment here I won't make it easy on you (😅) as I would like you to verify that for yourself as well. Do let me know when you'll figure out how to test these flows.

[neeldoshii]

Hi @ParaskP7
Follow up in the above tasks:-

1. This class WPBottomSheetDialogFragment doesn't have findViewById.

2. With the help of debugger I reproduced this class CollapseFullScreenDialogFragment.java. I think CommentFullScreenDialogFragment.kt should also be added in the task list for migrating to viewbinding.

[neeldoshii]

After investigating those real quick, I found out that both the RoleChangeDialogFragment and RoleSelectDialogFragment are indeed being used in the app(s). Thus we cannot (safely) remove them. Again, as per my comment #20933 (review) I won't make it easy on you (😅) as I would like you to verify that for yourself as well. Do let me know when you'll figure out how to test these flows.

Apologies over here, I was testing the app only for wordpress build variant and didn't knew anything about jetpack variant features of the app. Which caused me most of the dialog not able to access like plugins and all.

I was so confused where is Reader Notifications in the app for a while then I read the blog about jetpack, will take a look now with debugger and find the flow of it and figure out.

[ParaskP7]

👋 @neeldoshii and thanks!

This class WPBottomSheetDialogFragment doesn't have findViewById.

I guess you're right, this FrameLayout bottomSheetLayout = dialog.findViewById(com.google.android.material.R.id.design_bottom_sheet); usage can be ignored. I've now marked that as done, thanks! ✅

With the help of debugger I reproduced this class CollapseFullScreenDialogFragment.java. I think CommentFullScreenDialogFragment.kt should also be added in the task list for migrating to viewbinding.

Actually, I think we should only consider Java classes for now, since, as per what the title suggests, this all is about making Java classes null-safe. Thus, you'll notice that in general I haven't included any Kotlin classes on the issue description and its lists. 😊

[ParaskP7]

👋 @neeldoshii and please, don't apologize to me, I am just here to guide you and help you become the most effective contributor you can be. So, first, let me THANK YOU again for all the work you've been doing! 🙇

Apologies over here, I was testing the app only for wordpress build variant and didn't knew anything about jetpack variant features of the app. Which caused me most of the dialog not able to access like plugins and all.

Yea, this project is quite complicated to be honest, 2 apps, 3 product flavors, etc. However, I am not sure why you were not able to use the WordPress variant to test both, RoleChangeDialogFragment and RoleSelectDialogFragment. I actually used the WordPress variant for that matter, the People menu item and screen should be available for this app as well.

Let me know if you can't find this screen. 🤔

I was so confused where is Reader Notifications in the app for a while then I read the blog about jetpack, will take a look now with debugger and find the flow of it and figure out.

👍

[neeldoshii]

WordpresJalapenoDebug	JetpackJalapenoDebug	WordpresWasabiDebug

Yea, this project is quite complicated to be honest, 2 apps, 3 product flavors, etc. However, I am not sure why you were not able to use the WordPress variant to test both, RoleChangeDialogFragment and RoleSelectDialogFragment. I actually used the WordPress variant for that matter, the People menu item and screen should be available for this app as well.

Initially I was using WordpresJalapenoDebug as its default, through which I was not able to view any people, then I switched to JetpackJalapenoDebug which again doesn't have people in menu. But in WordpresWasabiDebug its reproducible.

Edit : The above WordpresWasabiDebugis of playstore jetpack. It's say try jetpack --> Redirects to playstore --> reproduced but can't test it.

Now my main doubt comes is when to use which flavour?

[ParaskP7]

👋 @neeldoshii !

Just a note that the screenshot is not that accurate, the WordpressWasabiDebug variant you point there has to actually be a Jetpack variant as it is green and looking Jetpack like.

Initially I was using WordpresJalapenoDebug as its default, through which I was not able to view any people, then I switched to JetpackJalapenoDebug which again doesn't have people in menu. But in WordpresWasabiDebug its reproducible.

So, the People screen is reproducible on both, the WordPress and Jetpack app, it is just that on the Jetpack app you need to click the ... More option to get to that My Site screen where the People menu item is available. For WordPress, this My Site screen is the default as it doesn't have a Dashboard screen, just like Jetpack has.

Does this make sense to you? 🙏

Edit : The above WordpresWasabiDebugis of playstore jetpack. It's say try jetpack --> Redirects to playstore --> reproduced but can't test it.

👍

Now my main doubt comes is when to use which flavour?

Both the Wasabi + Jalapeno variant are actually the same, used for debugging purposes, there is no practical difference there. So, try to use Jalapeno only, WordpressJalapeno for the WordPress app and JetpackJalapeno for the Jetpack app. This way you will avoid every variant related confusion.

[neeldoshii]

Just a note that the screenshot is not that accurate, the WordpressWasabiDebug variant you point there has to actually be a Jetpack variant as it is green and looking Jetpack like.

You are correct ✅. That was jetpack app from Playstore.

So, the People screen is reproducible on both, the WordPress and Jetpack app, it is just that on the Jetpack app you need to click the ... More option to get to that My Site screen where the People menu item is available. For WordPress, this My Site screen is the default as it doesn't have a Dashboard screen, just like Jetpack has.

I am not sure how ❓, but for both Jetpack & Wordpress Jalapeno(both debug version) I am not able to reproduce. the more option similar to jetpack app (playstore release version) & people in Wordpres version.

---

Several tasks require a WordPress account login, which is currently not possible for contributors due to recent changes in the API. Therefore, I am un-assigning myself so others can tackle the remaining classes who has login access.

[ParaskP7]

👋 @neeldoshii and I am very 😞 about you not being able to test this functionality, most probably due to the login access, my apologies for that! 🫂

I am un-assigning myself so others can tackle the remaining classes who has login access.

Makes sense to me, let's try and find you something else to work on! 🙏

[neeldoshii]

Hi @ParaskP7 👋,
I guess we have forgotten to tick the AddCategoryFragment, PostSettingsInputDialogFragment, CollapseFullScreenDialogFragment, PluginDetailActivity

[ParaskP7]

Done, thanks @neeldoshii ! 🥇