[Reader][0 blogs/0 tags] Fix Subscription feed infinite loading and fetch when tapping filters #20216
Conversation
IMPORTANT: this breaks ReaderPostListFragment/ViewModel fetch logic because it was relying on this bug causing FollowedTagsChanged to be triggered in the EventBus all the time. With this fix the Subscriptions feed is completely broken (and the other feeds that use this Fragments are likely to be broken as well, which are all the other except Discover).
The empty view is being reused for showing several messages, including the fetch message. Because of that, that are pieces of code that rely on previous code showing the "empty view" and just alter its content, without calling "show" again. If we hide the empty view when the list is actually empty, we get to a scenario of a blank screen when coming back to this feed from some other screens. Example: with 0 blog subscriptions, in the Subscriptions feed, going to the tag suggestions and back cause the Subscriptions feed to be blank.
Tag Interests has an infinite loading because a use case expects the event that is not triggered anymore (I think).
Upon investigation I realized that most places using this event were actually using it as a callback for the end of the fetch task rather than something that is ONLY emitted when the tags CHANGE, which was what the name implied. That only worked because of the bug mentioned in commit 3017bab that did not properly compare the local and server tags and considered all fetches as requiring a local database write, which in turn triggered the event. The constant triggering of that FollowedTagsChanged event was also the cause of bugs #20009 and #20010 since part of the code that runs there was actually only supposed to run some times. To fix that I renamed it to FollowedTagsFetched to better represent what it does and figured out which parts of the code inside `ReaderPostListFragment` event listener were supposed to run and in which scenario. I also added a `didChange` field in the `FollowedTagsFetched` event for completeness but that part might not be needed in the future, since the only code using that is legacy code related to the old subfilter which should be removed in the future.
|
 |
App Name |  Jetpack |
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr20216-c702921 | |
| Commit | c702921 | |
| Direct Download | jetpack-prototype-build-pr20216-c702921.apk |
|
 |
App Name |  WordPress |
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr20216-c702921 | |
| Commit | c702921 | |
| Direct Download | wordpress-prototype-build-pr20216-c702921.apk |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## trunk #20216 +/- ##
==========================================
- Coverage 40.31% 40.31% -0.01%
==========================================
Files 1469 1469
Lines 67672 67676 +4
Branches 11211 11209 -2
==========================================
+ Hits 27281 27282 +1
- Misses 37897 37900 +3
Partials 2494 2494 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks for the PR, @thomashorta . It's working as expected.
I'm approving since code LGTM but left a suggestion.
| @@ -26,16 +26,27 @@ private ReaderEvents() { | |||
| throw new AssertionError(); | |||
| } | |||
|
|
|||
| public static class FollowedTagsChanged { | |||
| public static class FollowedTagsFetched { | |||
| || displayNameUpdateWasNeeded | ||
| ) { | ||
| boolean didChangeFollowedTags = false; | ||
| if (!localTopics.isSameList(serverTopics)) { | ||
| AppLog.d(AppLog.T.READER, "reader service > followed topics changed " | ||
| + "updatedDisplayNames [" + displayNameUpdateWasNeeded + "]"); |
There was a problem hiding this comment.
Since the variable displayNameUpdateWasNeeded was removed from the if and is now only used in this AppLog, would it make sense to remove it? If so, perhaps we can even remove the displayNameUpdateWasNeeded method.
There was a problem hiding this comment.
Good point, I thought I had removed it but I think I forgot! Thanks for noticing that!
There was a problem hiding this comment.
Looks good to me. However, I noticed that we're retrieving the tags list and triggering the FollowTagsChanged event too many times, even when it seems unnecessary. The event is fired when tapping on the Tags pills to open the bottom sheet and also when tapping on the edit button in the bottom sheet. I'm wondering if we really need to sync tags with this frequency.
Do you mean the logic to fetch the tags, which ends up triggering now the For the examples you mentioned ("tapping the tag pills" and "edit button") I think those are actually very good examples that makes sense to keep fetching and syncing the tags, since they are the main entry points for screens that display the subscribed tags to the user, so in my opinion those scenarios need to keep fetching data from the backend. But like any other app/feature that has caching, the "fetch policy" is hard to get right and there's always a trade-off. Ex: using "cache-first" will return fast but might show stale data, while "network-only" might be slow but will guarantee we're synced with the backend. |
I don't completely agree with the part above. It's ok to fetch from the server when tapping |
|
Regarding CodeCov message above (#20216 (comment)), I don't think there's any action to be done, since none of those files were covered by unit tests, and adding those tests in this fix is out of scope. |
That's a good point! There's definitely room for improvement there in terms of fetching that should be investigated when we decide to refactor the business logic and architecture behind the Reader. |
thomashorta
deleted the
issue/20010-reader-ia-subscription-infinite-loading
branch
Fixes #20009
Fixes #20010
The biggest change in this PR is fixing the logic inside
ReaderUpdateLogicto avoid unnecessary DB writes which were happening always, even when the server tag list was the same as the local tag list, which in turn made theFollowTagsChangedstop being triggered every single time a fetch was done.Fixing that issue made the
FollowTagsChangedevent only be triggered when we, in fact, had changes in the local tag DB, which solved the issues referenced above BUT broke a lot of other things, since much of the existing code listening to that event was relying onFollowTagsChangedbeing triggered at the end of all tag fetching tasks.To solve that I renamed the
FollowTagsChangedtoFollowTagsFetchedand made it trigger in the same situation as it was before (regardless of changes being made to the local DB), added a new fielddidChangeto propagate the information if a change was done locally or not, and updated the logic inReaderPostListFragmentevent listener that caused the 2 bugs above to happen, by guarding with the specific conditions in which we really want fetching to happenTo Test:
Before running each test scenarios below make sure you have 0 blogs and 0 tags followed.
Infinite loading after subscribing to tags
feature/reader-iabranch buildTapping the blog/tag filter pills causes the empty list to refresh
feature/reader-iabranch buildRegression 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.txtif necessary.Testing Checklist: