-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fix discarded quick start notification observers #20997
Fix discarded quick start notification observers #20997
Conversation
📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
|
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.
Thanks for the work!
I made a few comments, most of them are about the same thing, usage of #selector
vs block observer
. As I understand, we could use selector observers to avoid explicit removal of observer in deinit
?
Regarding testing, I'm not sure how to test this change. Is Quick Start still an active feature? If so, how can I trigger it?
Yes, it is used. It can be triggered both with a new site and an existing site. You can check QuickStartFactory
to see use cases.
1 | 2 | 3 |
---|---|---|
✅ I tested and the quick start functionality continues to be working.
However, I noticed that:
deinit
is never called anywhere, even after logout. Memory debugger shows many VCs, Cells, Services continue to be initialized when only the login view is shown. Hard for me to imagine it's intentional, so I suspect more memory leaks are in playDashboardQuickActionsCardCell
is always initialized andstartObservingQuickStart
is called regardless if quick start is actually shown or not. Again, I suspect some issues in the dashboard implementation
I'll investigate a bit more and create issues if necessary
...sses/ViewRelated/Blog/Blog Dashboard/Cards/Quick Actions/DashboardQuickActionsCardCell.swift
Outdated
Show resolved
Hide resolved
...s/Classes/ViewRelated/Blog/Blog Dashboard/Cards/Quick Start/NewQuickStartChecklistView.swift
Outdated
Show resolved
Hide resolved
...ress/Classes/ViewRelated/Blog/Blog Dashboard/Cards/Quick Start/QuickStartChecklistView.swift
Outdated
Show resolved
Hide resolved
WordPress/Classes/ViewRelated/Blog/My Site/MySiteViewController+QuickStart.swift
Outdated
Show resolved
Hide resolved
WordPress/Classes/ViewRelated/Blog/My Site/MySiteViewController.swift
Outdated
Show resolved
Hide resolved
WordPress/Classes/ViewRelated/Blog/Site Picker/SitePickerViewController+QuickStart.swift
Outdated
Show resolved
Hide resolved
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 the changes! LGTM! ✅
Relates to #20994.
All the notification handlers in this PR use
weak self
reference. That means, even though the observer and the handler block is not being released and called when those notification are emitted, the actually code may not get executed ifself
has already been released.There are a few places where a
stopObservingQuickStart
function removes the wrong observer, which not only won't stop observing quick start, but also may incorrectly cause other notification handling code not get executed. However, I have checked all thestopObservingQuickStart
call sites, all of them are called from a deinit function, so this removing wrong observers code probably doesn't actually cause any issue at the moment.Regarding testing, I'm not sure how to test this change. Is Quick Start still an active feature? If so, how can I trigger it?
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:
RELEASE-NOTES.txt
if necessary.UI Changes testing checklist: N/A