Migrated `PostSettingsInputDialogFragment` to ViewBinding #20941

neeldoshii · 2024-06-06T12:01:04Z

Description

This pull request migrates the AddCategoryFragment to use ViewBinding, improving type safety and eliminating the need for findViewById calls.

Fixes

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

Screenshots/ Video

PostSettingsInputDialogFragment.webm

Steps to reproduce

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.

Testing Checklist (strike-out the not-applying and unnecessary ones):

ParaskP7

ParaskP7

👋 @neeldoshii !

Thank you so much for your contribution to JP/WPAndroid with this PR! 🥇

I have reviewed and tested this PR as per the instructions, along with triggering CI on it via a draft PR, which I just pushed on the main repo, everything works as expected, good job! 🌟

I have left couple of suggestions (💡) for you to consider. I am not going to approve this PR, but also, I don't want to block it, thus I'll just add a comment review. I am NOT going to merge this PR yet to give you some time to apply any of my suggestions.

WordPress/src/main/java/org/wordpress/android/ui/posts/PostSettingsInputDialogFragment.java

ParaskP7 · 2024-06-12T11:45:42Z

WordPress/src/main/java/org/wordpress/android/ui/posts/PostSettingsInputDialogFragment.java

@@ -94,34 +93,32 @@ public void onDismiss(DialogInterface dialog) {
    public Dialog onCreateDialog(Bundle savedInstanceState) {
        AlertDialog.Builder builder =
                new MaterialAlertDialogBuilder(new ContextThemeWrapper(getActivity(), R.style.PostSettingsTheme));
-        LayoutInflater layoutInflater = getActivity().getLayoutInflater();
+        LayoutInflater layoutInflater = requireActivity().getLayoutInflater();


Minor (🔍): Although this change is perfectly valid, consider avoid adding an unrelated change on a single commit. If you need to make this change, add a separate targeted commit on top of the main set of commits on a PR. Then, explaining the reasoning behind that change, even maybe group other such changes to make this change more impactful. How does this sound? 🙏

PS: No need for you to do anything here, this is just for educational purposes only.

ParaskP7 · 2024-06-12T12:27:22Z

Another tip from my side @neeldoshii , when you update your PR, enter into the habit of merging your changes with the latest change from trunk. This way, when I try to update my debug version of either the WordPress or Jetpack apps it won't make it hard on me, asking me to uninstall and install the app again, just because the version of the app on this PR is older (see version.properties).

Also, this way, you are making sure that new changes from trunk aren't affecting your changes and everything is tested with the most sync version of the app(s).

Does that make sense to you? 😊

FYI: As you can understand, uninstalling and installing the app, then trying to login before testing such small changes can reduce a developer's productivity while reviewing a PR.

neeldoshii · 2024-06-13T05:42:17Z

FYI: Btw, I am not sure why you are doing that, but feel free to add a separate commit with any change that is suggested during a PR review. Instead, if you keep rebasing your single commit it creates a problem where I also have to rebase the draft PR for CI. Also, in general, after any PR gets open, the guidance is to not rebase and force-push any more, as this makes it difficult to review the changes, as a review ideally would have to be done from scratch. Just adding extra commit on top is perfectly fine. Does that make sense to you? 🙏

Sure @ParaskP7, I understand. 👍 Previously, I was rebasing and squashing commits to keep the commit history clean and make it easier to revert changes if needed. However, I see your point about the difficulties it creates for reviewing and CI processes. From now on, I'll avoid rebasing after a PR is open and will add any changes as separate commits instead. Thanks for the guidance! 🙏

Another tip from my side @neeldoshii , when you update your PR, enter into the habit of merging your changes with the latest change from trunk. This way, when I try to update my debug version of either the WordPress or Jetpack apps it won't make it hard on me, asking me to uninstall and install the app again, just because the version of the app on this PR is older (see version.properties).

Understood! 👍

While merging the branch to upstream to the recent changes. I messed up which caused pushing the changes of commits behind from upstream. Can I perform git reset and force push now to clean this? What do you think is.better to fix this now?

ParaskP7 · 2024-06-13T07:12:15Z

👋 @neeldoshii and thanks for being so cooperative! 🙏

While merging the branch to upstream to the recent changes. I messed up which caused pushing the changes of commits behind from upstream. Can I perform git reset and force push now to clean this? What do you think is.better to fix this now?

Yes, no worries, please start over with a clean set. 👍

sonarcloud · 2024-06-13T10:48:00Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

ParaskP7 · 2024-06-17T07:58:15Z

👋 @neeldoshii !

Can I perform git reset and force push now to clean this? What do you think is.better to fix this now?

I am still waiting for you on that, correct? 🤔

neeldoshii · 2024-06-17T08:12:22Z

👋 @neeldoshii !

Can I perform git reset and force push now to clean this? What do you think is.better to fix this now?

I am still waiting for you on that, correct? 🤔

Hi @ParaskP7, I already did. Ready for review!

ParaskP7 · 2024-06-17T08:17:24Z

Hi @ParaskP7, I already did. Ready for review!

Awesome @neeldoshii , thanks for the heads-up, will do the review asap! 👍

ParaskP7

👋 @neeldoshii !

Looking at your updates I am not seeing you applying at least this suggestion of mine, changing mBinding to binding. Unfortuantely, I can't merge this as it is breaking our coding guidelines in general. Can you deal with it please? 🙏

neeldoshii · 2024-06-18T04:47:19Z

👋 @neeldoshii !

Looking at your updates I am not seeing you applying at least this suggestion of mine, changing mBinding to binding. Unfortuantely, I can't merge this as it is breaking our coding guidelines in general. Can you deal with it please? 🙏

Hi @ParaskP7, nice catch thanks. I have updated the requested changes.

ParaskP7 · 2024-06-18T07:45:14Z

Hi @ParaskP7, nice catch thanks. I have updated the requested changes.

Thanks @neeldoshii , will do the review asap! 👍

ParaskP7

🎉 @neeldoshii , everything is looking good, your second contribution to JP/WPAndroid is about to become history and soon merged into trunk, thanks so much for this work! 🥇

neeldoshii mentioned this pull request Jun 6, 2024

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

Open

ParaskP7 reviewed Jun 10, 2024

View reviewed changes

ParaskP7 self-requested a review June 10, 2024 10:35

ParaskP7 added Core Tech Debt labels Jun 10, 2024

ParaskP7 added this to the Future milestone Jun 10, 2024

ParaskP7 mentioned this pull request Jun 10, 2024

Migrated AddCategoryFragment to ViewBinding #20933

Merged

13 tasks

neeldoshii marked this pull request as draft June 10, 2024 18:48

neeldoshii force-pushed the Migrated-`PostSettingsInputDialogFragment`-to-ViewBinding branch from dc5b518 to 8528c20 Compare June 11, 2024 01:30

neeldoshii marked this pull request as ready for review June 11, 2024 01:30

neeldoshii force-pushed the Migrated-`PostSettingsInputDialogFragment`-to-ViewBinding branch from 3c62fbf to 57cfb46 Compare June 11, 2024 06:20

ParaskP7 mentioned this pull request Jun 11, 2024

[Do Not Merge] - CI Trigger for #20941 #20971

Merged

ParaskP7 reviewed Jun 11, 2024

View reviewed changes

WordPress/src/main/java/org/wordpress/android/ui/posts/PostSettingsInputDialogFragment.java Outdated Show resolved Hide resolved

WordPress/src/main/java/org/wordpress/android/ui/posts/PostSettingsInputDialogFragment.java Outdated Show resolved Hide resolved

neeldoshii force-pushed the Migrated-`PostSettingsInputDialogFragment`-to-ViewBinding branch from fb445cc to 0c09e9e Compare June 11, 2024 11:57

ParaskP7 reviewed Jun 12, 2024

View reviewed changes

ParaskP7 self-requested a review June 12, 2024 12:16

neeldoshii force-pushed the Migrated-`PostSettingsInputDialogFragment`-to-ViewBinding branch from 19499d6 to d73aa4d Compare June 13, 2024 09:29

Migrated PostSettingsInputDialogFragment to ViewBinding

3400278

neeldoshii force-pushed the Migrated-`PostSettingsInputDialogFragment`-to-ViewBinding branch from d73aa4d to 3400278 Compare June 13, 2024 09:31

Redundant : Extra Line Commit Removed

6a13711

ParaskP7 requested changes Jun 17, 2024

View reviewed changes

Changed mbinding to binding

48d7fdf

ParaskP7 self-requested a review June 18, 2024 08:42

ParaskP7 approved these changes Jun 18, 2024

View reviewed changes

ParaskP7 merged commit 3b1e418 into wordpress-mobile:trunk Jun 18, 2024
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrated `PostSettingsInputDialogFragment` to ViewBinding #20941

Migrated `PostSettingsInputDialogFragment` to ViewBinding #20941

neeldoshii commented Jun 6, 2024

ParaskP7 left a comment •

edited

Loading

ParaskP7 left a comment

ParaskP7 Jun 12, 2024

ParaskP7 commented Jun 12, 2024

neeldoshii commented Jun 13, 2024 •

edited

Loading

ParaskP7 commented Jun 13, 2024

sonarcloud bot commented Jun 13, 2024

ParaskP7 commented Jun 17, 2024

neeldoshii commented Jun 17, 2024 •

edited

Loading

ParaskP7 commented Jun 17, 2024

ParaskP7 left a comment

neeldoshii commented Jun 18, 2024

ParaskP7 commented Jun 18, 2024

ParaskP7 left a comment •

edited

Loading

Migrated PostSettingsInputDialogFragment to ViewBinding #20941

Migrated PostSettingsInputDialogFragment to ViewBinding #20941

Conversation

neeldoshii commented Jun 6, 2024

Description

Fixes

Screenshots/ Video

Steps to reproduce

PR Submission Checklist:

Testing Checklist (strike-out the not-applying and unnecessary ones):

ParaskP7 left a comment • edited Loading

Choose a reason for hiding this comment

ParaskP7 left a comment

Choose a reason for hiding this comment

ParaskP7 Jun 12, 2024

Choose a reason for hiding this comment

ParaskP7 commented Jun 12, 2024

neeldoshii commented Jun 13, 2024 • edited Loading

ParaskP7 commented Jun 13, 2024

sonarcloud bot commented Jun 13, 2024

Quality Gate passed

ParaskP7 commented Jun 17, 2024

neeldoshii commented Jun 17, 2024 • edited Loading

ParaskP7 commented Jun 17, 2024

ParaskP7 left a comment

Choose a reason for hiding this comment

neeldoshii commented Jun 18, 2024

ParaskP7 commented Jun 18, 2024

ParaskP7 left a comment • edited Loading

Choose a reason for hiding this comment

Migrated `PostSettingsInputDialogFragment` to ViewBinding #20941

Migrated `PostSettingsInputDialogFragment` to ViewBinding #20941

ParaskP7 left a comment •

edited

Loading

neeldoshii commented Jun 13, 2024 •

edited

Loading

neeldoshii commented Jun 17, 2024 •

edited

Loading

ParaskP7 left a comment •

edited

Loading