Migrate TimelineFragment and dependencies to new architecture #3436
Conversation
The layering of this code was weird. There's paging sources and remote
mediators making calls "up" in to the viewmodel, so they had a cyclic
dependency on each other.
And there's a remote mediator when there's no local database to cache to,
which made things more complicated.
So:
1. Remove the remote mediator
2. Put the code to fetch statuses in to the paging source
3. Push the a variant of the "TimelineKind" type in to the paging source
(probably not its final location), as this needs to be as low down the
dependency tree as possible to avoid cycle.
- Implement it as a sealed class instead of an enum, so that the
subclasses can hold non-null Strings where necessary, instead of relying
on `!!`
4. Comment out some code that either needs to be deleted or refactored, I'm
not sure which yet.
This builds and runs, and shows network timelines (local, federated, etc).
Some functionality it lost (e.g., retaining state about whether or not the
user has expanded a status) but that will be restored in future commits.
# Conflicts: # app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/CachedTimelineViewModel.kt # app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/NetworkTimelineViewModel.kt # app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/TimelineViewModel.kt
Use this in `NetworkTimelineViewModel`.
Move it from TimelineFragment to TimelinewViewModel. Update StatusDisplayOptions.kt to handle SHOW_CARDS_IN_TIMELINES pref
nikclayton
changed the title
The old type was an enum, the new type is a hierarchy of sealed classes. This allows each class to carry with it the information necessary for that timeline. For example, the PublicFederated timeline needs no additional data. But a timeline for a hashtag does. This simplifies TimelineFragment.newInstance(). Before the called had to know which optional parameters to pass in, or whether they should actually call newHashtagInstance. Now they just construct a timeline of the appropriate kind, with the correct data, and pass that in. This also means that initialisation of TimelineFragment does not need to unpack data from multiple extras in the launch intent, everything is in the parcelized TimelineKind.
# Conflicts: # app/src/main/java/com/keylesspalace/tusky/components/timeline/TimelineFragment.kt # app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/NetworkTimelinePagingSource.kt # app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/NetworkTimelineRemoteMediator.kt # app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/NetworkTimelineViewModel.kt
- Add CachedTimelineRepository between CachedTimelineViewModel and CachedTimelineRemoteMediator - Update CachedTimelineViewModel to use the repository - Modify TImelineFragment to be closer to NotificationsFragment (easier to figure out what code can be reused) - Add DeveloperTools to clear the DB, and remove the most recent 40 statuses - Add many TODOs marking code to refactor or remove
- Remove StatusViewData.Placeholder - Move StatusViewData.Concrete into StatusViewData
- Delete the class - Update instantiation locations - Remove obsolete string resource
CachedTimelineViewModel no longer needs an injected AppDatabase. To do this: - Extend what was `NotificationWorkerFactory` to `WorkerFactory`. This can construct arbitrary Workers as long as they provide their own Factory for construction. The per-Worker factory contains any injected components just for that worker type, keeping `WorkerFactory` clean. - Move `NotificationWorkerFactory` to the new model. - Implement `PruneCacheWorker`, and remove the code from `CachedTimelineViewModel`. - Create the periodic worker in `TuskyApplication`, ensuring that the database is only pruned when the device is idle.
This pushes the last of the EventHub responsibility out of TimelineFragment in to TimelineViewModel
Doesn't actually save it (is consistent with current behaviour) but provides the hook from which to hang the future code.
I will take a video. Perhaps the reason I'm having so much trouble with this is what I'm seeing is not how the patch is supposed to work. |
|
… okay, this video came out very confusing , one second |
|
Here is a demo video. It shows two things:
In this video, I show three separate copies of Tusky:
I made this video because @nikclayton said "The Notifications tab has been using this code, or a version of it, since v22.0, with no user complaints" [including from me], which is confusing because I noticed no change in the notifications tab when going from v21 to v23+andihacks whereas I see a very large difference going from v23+andihacks to this PR. What I found in the video was that the notification tab behavior of this PR is the same as vanilla v23.0, but that v23+andihacks has a notification tab behavior different from either, which should not be possible. Here is the video: https://www.youtube.com/watch?v=YvWRoj_LuiM 0:04: Boot Tusky v23.0. Let's focus on 1:15. There are two possible explanations here. One is that this is about the difference between v23 and v23+andihacks— this is possible, but I don't think this could be it, due to the minor nature of the patches. The other explanation, which I think is what is happening, is that there is a bug (which I coincidentally managed to trigger in my v23-andihacks test but not my v23-vanilla test) which can cause the notifications tab to "skip" a large amount of content. In other words, it is as if the "Show More" behavior was retained, but with a less consistent trigger condition, and without any "Show More" button. Although this bug is bad (losing user content without indication is very, very wrong!), this bug was what made it possible for me to use v23 at all, because apparently whenever I got too far behind in the feed I would just fiddle with it until I accidentally triggered the bug. I really want to stress this: If the behavior I saw in my vanilla v23 test above had been the behavior I had seen on first installing v23, I would not have switched from v21 to v23 at all, I would have stayed on v21. If we have not seen user complaints about the v23 behavior, it could be that users who get "too far behind" in the notifications tab are accidentally triggering this bug. (A third possibility is that changing filters, which I do pretty frequently, resets you to the top of the feed automatically. I haven't specifically tested this.) |
I think that because this patch changes the user model so significantly, we need to change more things to fit the new user model or the new user model is incoherent. (In other words I don't like the patch, but I think it could be usable as long as we take it significantly further.) I feel confident one of those the further changes should be "Invalidate the loaded-to position when Tusky is backgrounded and de-backgrounded". I think also probably the scrolled-to position should be invalidated when you change tabs/panes [including going to say, the preferences], and probably even it should be invalidated on a timer, say when ten seconds pass without user input. I also think that regardless of where the tab-tab scrolls you to, if that tab tap would result in a post load, tab-tapping should scroll you above the reload icon.
Okay, I don't think this is a very good library then :/ I assume there's some fourth operation that allows us to do in-situ post edits and post deletion on the timeline? Like, I assume the content isn't immutable, only the page structure? Can you elaborate on what the "discard" operation entails? How disruptive of it? Does it entail a screen flash? Does it require flushing our caches of data (I assume no, we are maintaining the cache of content and Paging3 is maintaining the cache of constructed UI objects and it is the cache of UI objects that clears on a "discard"/invalidate?) |
|
The bug you note on your andihacks branch at 1:15 -- unless you can reproduce that on v23 or this PR I'm not going to investigate. Given that your branch changes how filters are handled it's possible that gap is due to misconfigured filters. I've never seen it in v23 or on this PR, and no one has reported anything like it elsewhere. To the contrary, what this PR does do is fix an existing bug caused by the "Load more" behaviour where posts are missing from the timeline (Home, Local, Federated, etc), and there are external user complaints about that behaviour. The rest of the behaviour in the video on v23 this PR looks as intended. I do note two changes that might make some things more obvious to users.
No. The data is supposed to be immutable. Not all the existing Tusky code honours this (e.g., by sneaking in mutable data structures where the libraries expect immutable ones). Fixing this is also on my todo list, I suspect it's the source of subtle bugs.
A refresh or reload operation[2] discards the existing data (from memory, and from the backing database if there is one), and new data is fetched and cached. Then the new data is applied to the list via the list adapter. The adapter contains code to diff the old list and the new list, figure out what's changed, and perform the minimum number of screen updates to show the new data. There is no screen flash. Those changes either appear directly, or smoothly animate in (e.g., see what happens when you view a thread). For much more on this see https://developer.android.com/codelabs/android-paging#0. [1] Resume occurs both when Tusky returns to the foreground, or if the user switches tabs. [2] "refresh" is just "reload, but remember and restore my reading position". Think of them as "My local cache of the data is out of date, it must be replaced by up to date content from the remote server". |
Possible D: |
|
Emphasis mine:
Fetched from where? Like… all the way from the server? Could it be fetched from a secondary data store Tusky hosts (so that if we wanted to do "correct" post edits or "load more" page inserts, we could simply change the Tusky device-side store, invalidate, and let paging3 redraw with amended content)? Or is the idea that having switched to paging3, "the data store" is now hosted entirely by paging3 and we no longer have a post store it could be drawn from? |
Yes. The on-device copy is treated as a local cache that can be discarded when necessary.
The word "simply" in that sentence is doing an awful lot of work. In practice, the answer is no. https://developer.android.com/topic/libraries/architecture/paging/v3-overview has more information on how the library is supposed to be use.
No. The data store (e.g., the local database) is still managed by the app. But on receipt of a "refresh" operation the local copy of the data is cleared, and a fresh copy fetched from the server. This does not mean all the local data is cleared, only the data that the server hosts. So information like "The user has clicked 'show more' on this status" and it should be rendered like that the next time it's displayed, or "This status has a content warning and the user has clicked to show the content of the post" is local view state that the app should maintain and not delete when the user refreshes. Tusky is currently inconsistent about doing that, as I think I mentioned earlier. I've got ideas about how to fix that, but they're orthogonal to this PR. |
|
Okay. Thank you for all your detailed explanations. However,
Based on a few of your comments but particularly this one, my personal opinion at this point is that paging3 is not appropriate for our use case and we should not be using it, possibly not even for notifications. |
@mcclure That ship sailed in Jan 2021, with #2238 Reading that PR, I'm somewhat amused to note that @charlag identified the mutable data problem and suggested a cleaner way to implement it in #2238 (comment). The approach that was used (#2238 (comment)) is some of the technical debt that needs to be cleaned up. |
This is because the progress spinner that indicates if a previous or next page is being loaded is a When being shown, this will wait 500ms before appearing. If there's a "hide" in the intervening 500ms then it won't show at all, preventing brief UI adjustments. Similarly, once it's showing it will stay shown for at least 500ms even if it's told to "hide" before then. Again, to avoid very brief UI adjustments. So if the load and processing of the previous (or next) page takes < ~500ms you'll not see a progress spinner. If it takes longer you will, and the spinner will be visible for at least 500ms. This is similar behaviour to what happens when viewing a thread. If you open a thread there's a network request to fetch the status you're viewing, and a second request to fetch the statuses above/below it. If those requests complete in < ~500ms you'll never see the loading indicator, they'll smoothly animate in. If those requests take longer you'll see a loading indicator, so you know there's something to wait for. |
…agment-arch # Conflicts: # app/build.gradle
I've been trying a build with that for the last 24 hours or so. I think it's too disruptive, as things that you would not expect to cause a refresh do. For example, tapping a message to view its thread and then pressing back will trigger a refresh. So will tapping media in a status to view it full screen (image, video, or audio). This means:
I think the user can expect this if they explicitly triggered a refresh -- reopening the app, swiping, choosing "refresh" from the menu. But I don't think that activities like "viewing a thread" or "opening an image" (or "swiping to this tab") have this expectation. I'll look and see if it's possible to do this only when the user is resuming the fragment because they've clicked on the tab to switch to it. |
I'm sorry, I think I was unclear. I was not trying to suggest we should refresh on returning to the program, which probably would be disruptive, but rather that the "water line" above which we do not load further data without user input (assuming I understand correctly and this "water line" exists) should be invalidated, and if the user is already scrolled close enough to the top of the newest page we would (in the absence of the "water line") begin loading in newer data (but not scrolling toward it, which would indeed be disruptive). |
The "waterline" your describing is only created when Tusky has requested the page and the server has returned an empty page (which is how the server says "This page you have requested does not exist"). Then, and only then, is a "waterline" created. To get Tusky to ask the server again you have to "refresh". That's exactly the same behaviour as the current code -- when you refresh in Tusky at the moment the newest page is explicitly loaded. You can not scroll up past this page without performing a refresh. Let's say you left Tusky reading page 90 of your home timeline, and have returned. In the intervening time pages 91-100 have been created on the server. In Tusky 23.0
In this PR
In both cases what happens when the user reaches the top of page 100 is the same -- the user can't scroll without a refresh. This PR improves things in the case where new pages become available while you're scrolling and haven't reached the top. Consider this slight change in the same scenario, where the server is adding new pages to the top of the timeline. In Tusky 23.0
In this PR
So typically, with this PR you will:
|
- Calculate itemsBefore and itemsAfter, get correct anchorPosition - Updating peeking and "show / hide the progress indicators" algorithm
…to the adapter at the time of the check
…aded in to the adapter at the time of the check" This reverts commit 5f8e8a9. This seems to work better with the new prepend/refresh code
Previous code started/stopped on the fragment lifecycle. So if you left the fragment the UI code would continue running (collecting from flows, ticking to update an invisible UI, etc).
…agment-arch # Conflicts: # app/lint-baseline.xml # app/schemas/com.keylesspalace.tusky.db.AppDatabase/53.json # app/src/main/java/com/keylesspalace/tusky/TabData.kt # app/src/main/java/com/keylesspalace/tusky/TuskyApplication.kt # app/src/main/java/com/keylesspalace/tusky/db/AppDatabase.java # app/src/main/java/com/keylesspalace/tusky/di/AppModule.kt # app/src/main/java/com/keylesspalace/tusky/settings/SettingsConstants.kt
Follow the same architecture as Notifications:
Other code changes to make this easier:
Fixes #3195, #3632, #3433, #3320, #3317, #3269, #2831, #2702, #3505, #3461, #3818