This repository was archived by the owner on Jul 23, 2024. It is now read-only.
Ema-289 library screen performance - DO NOT MERGE #292
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a placeholder PR, to show some progress on the library screen performance issue.
@orionthewake I've done some research and some progress on the #289.
The current data flow implementation is extremely complicated and a bit too tightly coupled. Instead of having reusable and clean way to differentiate different types of data fetching, the paging library is just deeply rooted at the data fetching, meaning it's not just easily refactored and changed to be database-first/offline-first.
I think, if we want to support full-on offline mode, we'd have to refactor a good amount of the application and the data flow, to somehow integrate database data instead of using the remote API as its primary source of information.
Also, all the adapters are using the same
ContentAdapter
, meaning that we could technically use this heavy coupling & system of data loading, and somehow try to switch from the remote source to a local source.That being said, I think a good estimate on this could be to spend a few weeks, instead of a week, to fully refactor all parts of the app to try to cache data locally before showing it.
I feel like I'd also like to refactor the UI parts and how the uiState handling is done, as that part seems also very convoluted and hard to work around/with.
I've attached a few screenshots of the behavior on this branch. The process is the following:
The issues with current data loading is the following:
loadInitial()
,loadAfter()
andloadBefore()
. This means we can load the initial page, and then move up or down (load after or before the initial page) in terms of data.The problem here is that we store all the content in the same way. Well it's all content, and all within the
Contents
table. So things like bookmarks, progressions, collections, etc. are all in the same table.That being said, in case we want to switch to an
offline-first
model, we'd have to decide how this is going to behave. How many items will we store, what happens when the user connects to the app - do we clear the DB and start loading and storing new items etc.There are many use cases we'd have to think about and develop/test because small things can mess up the paging setup and the DB contents.
In an ideal (and much simplified use case), we'd just connect our data to a LiveData from the DB and then as we scroll we'd load more and more items from the network. That way the data would just update automatically.
But as it sits, the data comes from a paging source and we'd have to refactor a good amount of code that's used all over the place.
We can discuss this more in our meeting but I feel like we'd have to do a bigger overhaul of the app soon to make things easier in the future!
These images show how the offline first behavior currently works. There is some data cached, but not all of it. I'd love to have this one of the bigger features we build in the next few months or something!