Fix crash in ReaderActivityLauncher

[aditi-bhatia]

Fixes #20847

This PR addresses a crash related to context being null when calling buildShowReaderCommentsIntent().

---

To Test:

I haven't been able to reproduce this crash, so code check + smoke test comments on the Reader.

---

Regression Notes

1. Potential unintended areas of impact

   • None

2. What I did to test those areas of impact (or what existing automated tests I relied on)

   • None

3. What automated tests I added (or what prevented me from doing so)

   • None

---

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):

• 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)

[wpmobilebot]

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

Note: Google Login is not supported on these builds.

[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	915584c
	Direct Download	jetpack-prototype-build-pr20848-915584c.apk

Note: Google Login is not supported on these builds.

[codecov]

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 ⚠️
...ress/android/ui/reader/ReaderActivityLauncher.java	0.00%	1 Missing ⚠️

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              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[irfano]

@aditi-bhatia, would you like to fix this using NonNull annotations? There are already warnings for parameters that don't have NonNull or Nullable annotations in the Java classes. When you add the NonNull annotation, other places that call these functions will show errors, and it also prevents calling this function with nullable parameters in the future.

For more info: There is an internal project (paaHJt-5fa-p2) for nullability annotations that we can assist with.

[aditi-bhatia]

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 👍

[irfano]

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.

[aditi-bhatia]

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.

[irfano]

I left two nitpicking comments and one for minor request to add the NonNull annotation. It was like a puzzle to find all possible instances of null parameters. 😅 Thanks for your effort! I'm approving, and you can merge after considering my comments.

[sonarcloud]

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

[aditi-bhatia]

Thanks for the review @irfano! I addressed the comments you made above and will go ahead and merge.

[sentry-io]

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

• ‼️ IllegalStateException: actionable_empty_view must not be null org.wordpress.android.ui.notifications.Notifica... View Issue

_{Did you find this useful? React with a 👍 or 👎}