Comment refactor Part 1

jarvislin commented 2 weeks ago

Currently, there are two entry points to the screen of comments:

Notifications List -> Comment
My Site -> Comments List -> Comment

For some reason, we implemented every thing in that big CommentDetailFragment. There is some logic that is only triggered by specific entry points. To maintain the simplicity of the parent class, I suggest relocating this code to a subclass. Furthermore, converting subclasses to Kotlin can enhance development efficiency.

This a PR contains:

Create two Fragments for different purpose.
Convert a file to Kotlin.
Refactor a .xml file for better performance

There will be follow-up PRs for furthering refactors.

To Test:

Sign in JP app
Go to the Notifications tab
Click on Comment type of notification
It should not occur crash issues.
Go to My Site -> Comments, click on a comment
It should not occur crash issues.
Done, thank you.

Regression Notes

Potential unintended areas of impact
- notifications, comment
What I did to test those areas of impact (or what existing automated tests I relied on)
- manual
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)

dangermattic commented 2 weeks ago

	1 Warning
:warning:	This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by :no_entry_sign: Danger

wpmobilebot commented 2 weeks ago

📲 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	pr20726-4ae36fa
	Commit	4ae36fa7d4d5d002654f0a069ec4c2d1d93a8463
	Direct Download	`wordpress-prototype-build-pr20726-4ae36fa.apk`

Note: Google Login is not supported on these builds.

wpmobilebot commented 2 weeks ago

📲 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	pr20726-4ae36fa
	Commit	4ae36fa7d4d5d002654f0a069ec4c2d1d93a8463
	Direct Download	`jetpack-prototype-build-pr20726-4ae36fa.apk`

Note: Google Login is not supported on these builds.

codecov[bot] commented 2 weeks ago

Codecov Report

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

Project coverage is 40.64%. Comparing base (2a1b240) to head (91063a2).

:exclamation: Current head 91063a2 differs from pull request most recent head 4ae36fa. Consider uploading reports for the commit 4ae36fa to get more accurate results

Files	Patch %	Lines
...press/android/ui/notifications/blocks/NoteBlock.kt	0.00%	1 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## feature/notifications_refresh_p2 #20726 +/- ## ================================================================= Coverage 40.64% 40.64% ================================================================= Files 1488 1488 Lines 68414 68411 -3 Branches 11344 11343 -1 ================================================================= Hits 27805 27805 + Misses 38088 38085 -3 Partials 2521 2521 ```

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

jarvislin commented 1 week ago

Thank you so much, Jarvis, for extracting fragments for notification and site comment details. Great work! 🎸

Since the original CommentDetailFragment is really large, I would encourage you to leave it in Java and convert those parts to Kotlin that are smaller and testable such as the brand-new fragments you extracted. I see a lot of not-null assertions that could potentially break the logic and it is worth migrating all of the sensitive parts gradually.

What do you think about that?

Yeah that makes sense to me, I also feel that directly converting the large file is too ambitious, thanks for the review 👍