-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactors Notes
to Kotlin
#20200
Refactors Notes
to Kotlin
#20200
Conversation
Generated by 🚫 Danger |
📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## feature/notifications_refresh_p1 #20200 +/- ##
====================================================================
+ Coverage 40.13% 40.16% +0.02%
====================================================================
Files 1470 1470
Lines 67837 67787 -50
Branches 11238 11240 +2
====================================================================
Hits 27228 27228
+ Misses 38112 38062 -50
Partials 2497 2497 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for tackling this! I've taken an early look while it is still in draft, and left a few comments. Please don't consider the suggestions as necessary for this Kotlinization PR, though, since the diff is already very large. With such a large class, it may make more sense to take a few steps toward migrating it.
private var mActions: JSONObject? = null | ||
private var mLocalStatus: String? = null | ||
|
||
private val mSyncLock = Any() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I could tell, we the current synchronized code doesn't require reentrancy. I wonder whether we could use a Mutex
here instead? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I experimented a bit with using Mutex
but this would require introducing coroutines and changes outside the scope of the Note
model. Tbh I'm not sure if locking is needed in all cases when accessing the json object 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@antonis Thank you for converting the Note
class to Kotlin. It's really helpful for us.
I've smoke tested the notification tab, the result is as expected, LGTM!
Description
This PR:
Notes
model to KotlinTo Test:
Regression Notes
Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)
PR Submission Checklist:
RELEASE-NOTES.txt
if necessary.Testing Checklist: