WordPress-Android Fix crash in ReaderActivityLauncher

Fixes #20847

To Test:

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)
- None
What automated tests I added (or what prevented me from doing so)
- None

PR Submission Checklist:

[x] I have completed the Regression Notes.
[x] I have considered adding accessibility improvements for my changes.
[x] 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):

[ ] ~WordPress.com sites and self-hosted Jetpack sites.~
[ ] ~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)~

May 18 '24 02:05 aditi-bhatia

📲 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	pr20848-915584c
	Commit	915584c756dc3f2fd311be49cc5ddbc77ddf76e0
	Direct Download	`wordpress-prototype-build-pr20848-915584c.apk`

Note: Google Login is not supported on these builds.

May 18 '24 03:05 wpmobilebot

📲 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	pr20848-915584c
	Commit	915584c756dc3f2fd311be49cc5ddbc77ddf76e0
	Direct Download	`jetpack-prototype-build-pr20848-915584c.apk`

Note: Google Login is not supported on these builds.

May 18 '24 03:05 wpmobilebot

Codecov Report

Attention: Patch coverage is 0% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 40.66%. Comparing base (f83cca4) to head (915584c). Report is 443 commits behind head on trunk.

Files	Patch %	Lines
.../ui/notifications/NotificationsListFragmentPage.kt	0.00%	8 Missing :warning:
...ress/android/ui/reader/ReaderActivityLauncher.java	0.00%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##            trunk   #20848      +/-   ##
==========================================
- Coverage   40.67%   40.66%   -0.01%     
==========================================
  Files        1490     1490              
  Lines       68631    68634       +3     
  Branches    11342    11344       +2     
==========================================
  Hits        27913    27913              
- Misses      38197    38200       +3     
  Partials     2521     2521

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

May 18 '24 03:05 codecov[bot]

Hi @irfano, thanks for the suggestion, the lack of annotations in the codebase was also something that I was concerned about. I spent some time looking into adding NotNull annotations here today and just wanted to double check this part:

When you add the NonNull annotation, [...] prevents calling this function with nullable parameters in the future.

From what I understand, NotNull only provides a warning if you pass a nullable parameter (which is good for code quality / ease of understanding), but doesn't actually prevent null from being passed. For example in the code below, even though context is annotated NotNull, a warning is displayed on line 200 but the code still runs and throws a NPE:

So from what I understand, adding NotNull annotations will be good for code hygiene but null checks are still required at some point to prevent an NPE, so I think it would be best to add both. What do you think? Let me know if I missed something 👍

May 22 '24 05:05 aditi-bhatia

I made the same change as you (added NonNull to lines 196 and 200), and but I received an error in NotificationsDetailListFragment.kt couldn't build. I'm not sure why you didn't get the same error. Probably java files show a warning while Kotlin files show errors.

but null checks are still required at some point to prevent an NPE

Yes! NonNull annotations don't provide fixes, but they help us find the deepest code where nulls are coming from. Eventually, we will need null checks to those places.

May 22 '24 13:05 irfano

Hi @irfano, thank you for the input 👍 I've added NonNull annotations as well as null checks in the calling functions if you'd like to take another look.

May 24 '24 19:05 aditi-bhatia