-
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
[Tags Feed] Fix duplicate tracking in Reader subfilter #20844
[Tags Feed] Fix duplicate tracking in Reader subfilter #20844
Conversation
Also making sure it works for Fragment recreation by the system (such as screen rotation) and ViewModel destruction due to owner being destroyed.
📲 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: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/tags-ia #20844 +/- ##
===================================================
+ Coverage 40.84% 40.88% +0.04%
===================================================
Files 1493 1493
Lines 68766 68770 +4
Branches 11350 11350
===================================================
+ Hits 28087 28117 +30
+ Misses 38131 38098 -33
- Partials 2548 2555 +7 ☔ 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.
LGTM!
While testing this PR i've found a bug in the top menu. We will discuss it on Slack before opening a new ticket for it.
This reverts commit 4855ec7.
I tested the scenarios we discussed on Slack and indeed the solution here doesn't seem to be working correctly for some of them. I'm marking the PR as |
This makes sure that the Lifecycle and ViewModel Owners handle everything correctly when observing LiveData, which avoids duplicate observers and allows for correct auto removal/re-observation due to lifecycle and owner state change.
@daniloercoli, to fix the issue I needed to radically change the solution. The new solution properly uses the observable mechanism by creating a local reference to the Observers, which prevents duplicate observation by default (thanks to the "Observer, LifecycleOwner" tuple being stable and consistent now), and also handles lifecycle and view model state changes automatically. Please review again with the new changes! |
WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderFragment.kt
Show resolved
Hide resolved
Tested and cannot replicate the issue anymore! Code looks robust, and adheres to good practices with regard to ViewModel and lifecycle management. LGTM! |
Feel free to merge this PR, once the problems with testes are resolved. |
…ss-Android into issue/20814-reader-subfilter-tracking-bug
Quality Gate passedIssues Measures |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Fixes #20814
reader_filter_sheet_item_selected
when selecting a filterable feed (e.g.: Subscriptions and Your Tags)To Test:
App Setting
->Privacy
->Collect Information
is enabledreader_tags_feed
Feature config is enabled (Debug Settings
->Remote Features
reader_filter_sheet_item_selected
is not trackedreader_filter_sheet_item_selected
is tracked ONLY ONCERegression 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)
onCleared
)reader_filter_sheet_item_selected
when opening a filterable feedPR Submission Checklist:
RELEASE-NOTES.txt
if necessary.Testing Checklist (strike-out the not-applying and unnecessary ones):