[Nullability Annotations to Java Classes] Replace `findViewById` with `ViewBinding` for History (`relatively safe`) #19172

ParaskP7 · 2023-09-13T14:08:57Z

This PR's main focus is to replace findViewById with ViewBinding for the HistoryDetailContainerFragment.java class.

Additionally, this PR is also dealing with adding any missing nullability annotations to this fragment class, focusing mainly on its fields, and, any effected neighbour classes. But first, all existing warnings on this class are being resolved/suppressed so as to make it easier to deal with any missing nullability checks (with help from the IDE), especially when a @Nullable annotation is added.

FYI: This change is relatively safe, meaning that although there are compile-time changes associated with this change, and it needs testing, the changes included in this PR are very targeted and specific to one screen, and one screen only. Thus, if this screen is tested appropriately, it should be safe to merge this to trunk.

PS: @RenanLukas I added you as the main reviewer, randomly so, since I just wanted someone from the Jetpack/WordPress mobile team to be aware of and sign-off on that change for JP/WPAndroid. Feel free to merge this PR directly yourself if you deem so.

findViewById -> ViewBinding

Nullability Annotation List:

Warnings Resolution List:

Warnings Suppression List:

Suppress deprecation warnings on history detail

To test:

Quickly smoke test any history related functionality on both, the WordPress and Jetpack apps, and see if everything is working as expected.
In addition to the above smoke test, you can expand the below and follow the inner and more explicit test steps within:

History Details Screen [HistoryDetailContainerFragment.java]

ℹ️ This test applies to both, the Jetpack and WordPress app.

Go to Posts screen and tap on any post.
Find the options menu (top-right) and tap on History .
From the History screen, tap on any revision.
Verify that the History Details screen is shown and functioning as expected.

Regression Notes

Potential unintended areas of impact
- N/A
What I did to test those areas of impact (or what existing automated tests I relied on)
- See To test section above.
What automated tests I added (or what prevented me from doing so)
- N/A

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)

@OverRide

This is done in order to avoid using the 'if (mBinding != null) { ... }' check everywhere and only use that where absolutely necessary, which usually is only needed when a 'binding' is required via an SDK related '@OverRide' method, that is, it being the 'binding' starting point. FYI: This way, when this class gets converted to Kotlin (in the future), instead of using the 'binding' as a parameter to a method, the same method will get converted to a 'binding' extension function, and then, everything will become much more straightforward. For what is worth, this pattern is already used as such with Kotlin classes. For example, see 'PreviewImageFragment.kt' class and its 'initializeViews()' extension function.

Warnings: - "'setHasOptionsMenu(boolean)' is deprecated" - "Overrides deprecated method in 'androidx.fragment.app.Fragment'" - "'androidx.fragment.app.FragmentStatePagerAdapter' is deprecated" - "'FragmentStatePagerAdapter(androidx.fragment.app.FragmentManager)' is deprecated "

Warning: "Inner class 'HistoryDetailFragmentAdapter' may be 'static'"

Warning: "Anonymous new Runnable() can be replaced with lambda"

FYI: 'nl-a' stands for 'nullable annotation'.

wpmobilebot · 2023-09-13T14:25:11Z

📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.

	App Name	Jetpack
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr19172-724f91b
	Commit	`724f91b`
	Direct Download	`jetpack-prototype-build-pr19172-724f91b.apk`

Note: Google Login is not supported on these builds.

wpmobilebot · 2023-09-13T14:25:59Z

📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.

	App Name	WordPress
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr19172-724f91b
	Commit	`724f91b`
	Direct Download	`wordpress-prototype-build-pr19172-724f91b.apk`

Note: Google Login is not supported on these builds.

RenanLukas

Thanks for the PR, @ParaskP7. I've tested WP and JP and everything is working as expected 🎉

I've left a small comment about the mBinding null check. Please let me know if it makes sense.

RenanLukas · 2023-09-18T23:24:19Z

WordPress/src/main/java/org/wordpress/android/ui/history/HistoryDetailContainerFragment.java

-        resetOnPageChangeListener();
-
-        return rootView;
+        return mBinding.getRoot();


💡 We're checking if mBinding is null on line 115 but we're using it here without checking it. I wonder if we should do something here if mBinding is null (e.g. throwing an exception) or if we can just drop the null check in line 115 🤔

Yeaaa, and thanks for this comment @RenanLukas , ideally I would like to drop the null check on line 115. Unfortunately, when I do that, for some reason, the IDE is warning us about it, I just doesn't understand that mBinding is already set, just a few lines above: Dereference of 'mBinding' may produce 'NullPointerException'

Also, when I do that, that is, dropping the null check on line 115, the IDE now starts warning us about line 163, as expected, I don't know why it is warning us on that already, probably an IDE bug of some kind: Method invocation 'getRoot' may produce 'NullPointerException'

🤷 🤷 🤷

I ended-up choosing to throw an IllegalStateException and guard the mBinding.getRoot() call. Let me know what you think and again, thanks for this suggestion:

This is done here: 724f91b

This is done in order to include the 'mBinding.getRoot()' return within the 'mBinding' nullability check and avoid getting a NPE on it, just like it is already done for every 'mBinding' related call above. PR Comment: https://github.com/wordpress-mobile/WordPress-Android/ pull/19172#discussion_r1329379578

ParaskP7 · 2023-09-19T09:06:22Z

Thanks for reviewing and testing this @RenanLukas , much appreciated! 🙇 ❤️ 🚀

RenanLukas

Thanks for updating the PR, @ParaskP7. LGTM

Considering the PR instructions I'm going to merge it to trunk.

ParaskP7 · 2023-09-19T13:49:38Z

Thank you for merging @RenanLukas ! 🙇 ❤️ 🚀

ParaskP7 added 10 commits September 13, 2023 17:06

Refactor: Replace findviewbyid with viewbinding for history detail

85f6586

Analysis: Add missing nullability annotations to sdk override methods

7c20443

Analysis: Make inner history detail fragment adapter class static

16d535f

Warning: "Inner class 'HistoryDetailFragmentAdapter' may be 'static'"

Analysis: Replace anonymous class with lambda for history detail

5e643b5

Warning: "Anonymous new Runnable() can be replaced with lambda"

Analysis: Remove adapter from field and use it as non-null

e28be1d

Analysis: Remove revisions from field and use it as nullable

01bb24e

Analysis: Add missing nl-a to on page change listener field

e811261

FYI: 'nl-a' stands for 'nullable annotation'.

Analysis: Add missing nl-a to on revision field

102dc63

FYI: 'nl-a' stands for 'nullable annotation'.

ParaskP7 added Core Tech Debt labels Sep 13, 2023

ParaskP7 added this to the Future milestone Sep 13, 2023

ParaskP7 self-assigned this Sep 13, 2023

ParaskP7 requested a review from RenanLukas September 13, 2023 14:31

ParaskP7 marked this pull request as ready for review September 13, 2023 14:31

ParaskP7 mentioned this pull request Sep 13, 2023

Nullability Annotations to Java Classes - Activity & Fragment - Replace findViewById with ViewBinding #18906

Open

ParaskP7 changed the title ~~[Nullability Annotations to Java Classes] Replace findViewById with ViewBinding for History (relatively safe)~~ [Nullability Annotations to Java Classes] Replace findViewById with ViewBinding for History (relatively safe) Sep 14, 2023

RenanLukas reviewed Sep 18, 2023

View reviewed changes

RenanLukas approved these changes Sep 19, 2023

View reviewed changes

RenanLukas merged commit c25f975 into trunk Sep 19, 2023

RenanLukas deleted the refactor/replace-findviewbyid-with-viewbinding-for-history branch September 19, 2023 13:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Nullability Annotations to Java Classes] Replace `findViewById` with `ViewBinding` for History (`relatively safe`) #19172

[Nullability Annotations to Java Classes] Replace `findViewById` with `ViewBinding` for History (`relatively safe`) #19172

ParaskP7 commented Sep 13, 2023 •

edited

Loading

wpmobilebot commented Sep 13, 2023 •

edited

Loading

wpmobilebot commented Sep 13, 2023 •

edited

Loading

RenanLukas left a comment

RenanLukas Sep 18, 2023

ParaskP7 Sep 19, 2023

ParaskP7 commented Sep 19, 2023

RenanLukas left a comment

ParaskP7 commented Sep 19, 2023

[Nullability Annotations to Java Classes] Replace findViewById with ViewBinding for History (relatively safe) #19172

[Nullability Annotations to Java Classes] Replace findViewById with ViewBinding for History (relatively safe) #19172

Conversation

ParaskP7 commented Sep 13, 2023 • edited Loading

To test:

Regression Notes

PR submission checklist:

UI Changes testing checklist:

wpmobilebot commented Sep 13, 2023 • edited Loading

wpmobilebot commented Sep 13, 2023 • edited Loading

RenanLukas left a comment

Choose a reason for hiding this comment

RenanLukas Sep 18, 2023

Choose a reason for hiding this comment

ParaskP7 Sep 19, 2023

Choose a reason for hiding this comment

ParaskP7 commented Sep 19, 2023

RenanLukas left a comment

Choose a reason for hiding this comment

ParaskP7 commented Sep 19, 2023

[Nullability Annotations to Java Classes] Replace `findViewById` with `ViewBinding` for History (`relatively safe`) #19172

[Nullability Annotations to Java Classes] Replace `findViewById` with `ViewBinding` for History (`relatively safe`) #19172

ParaskP7 commented Sep 13, 2023 •

edited

Loading

wpmobilebot commented Sep 13, 2023 •

edited

Loading

wpmobilebot commented Sep 13, 2023 •

edited

Loading