-
-
Notifications
You must be signed in to change notification settings - Fork 446
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
feat: Improve new user experience + adjust home load #5491
Conversation
If you agree with the data binding approach, I would like to mention that for the future it would be very beneficial to have a repository layer. At the moment view models make their calls directly on the Retrofit instances and the state is being saved either on LiveData at the view model level - e.g. SubscriptionsViewModel (this is already a great step, however does not support exchange of information between screens), or directly in fragments (usually the data is re-fetched when we re-enter the screen). Here's a very simplified example of what it would look like: class HomeFragment: Fragment() {
private val viewModel: HomeViewModel by activityViewModels()
override fun onResume() {
super.onResume()
viewModel.feedContent.observe(viewLifecycleOwner, ::showFeed)
}
override fun onPause() {
super.onPause()
viewModel.feedContent.removeObserver(::showFeed)
}
...
}
class HomeViewModel(
private val contentRepository: IContentRepository
): ViewModel() {
val feedContent get() = contentRepository.feedContent
init {
viewModelScope.launch {
contentRepository.loadFeed()
}
}
}
class ContentRepository: IContentRepository {
override val feedContent: LiveData<List<StreamItem>?> get() = _feedContent
private val _feedContent: MutableLiveData<List<StreamItem>> = MutableListData(null)
override fun loadFeed() {
if (feedContent.value == null) {
// Load from API and fill live data
}
}
} With this approach, all data fetched in one screen would be available in other screens that need the same information. All this management would be done inside the repositories |
app/src/main/java/com/github/libretube/ui/models/HomeViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/github/libretube/ui/models/HomeViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/github/libretube/ui/fragments/HomeFragment.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/github/libretube/ui/fragments/HomeFragment.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/github/libretube/ui/fragments/HomeFragment.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/github/libretube/ui/fragments/HomeFragment.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/github/libretube/ui/activities/SettingsActivity.kt
Outdated
Show resolved
Hide resolved
…nt.kt Co-authored-by: Bnyro <[email protected]>
…nt.kt Co-authored-by: Bnyro <[email protected]>
…nt.kt Co-authored-by: Bnyro <[email protected]>
Co-authored-by: Bnyro <[email protected]>
Thanks a lot for the review Bnyro 💪 I hope I have not forgotten any topic. |
app/src/main/java/com/github/libretube/ui/models/HomeViewModel.kt
Outdated
Show resolved
Hide resolved
Personally, I don't think we need that additional level of abstraction. I agree that refactoring parts of the app to make more use of view models to persist data is the right direction, but these additional layers would just cause a lot of additional code to maintain. It might be a common practice in the industry (I can't tell, I don't work there), but I think for us it's much better to keep things as short and simple as possible to keep maintenance as easy as possible. We can use the same view model at different fragments to share data, which already works very well. Hence we should only use such repository approaches if really needed, and I don't currently see a place in the app where it'd be really useful. |
* Change buttons style for consistency; * Move updateIfChanged to a separate file;
Great 🙌 I hope I addressed all points! Regarding the repositories, I agree that at the moment it might not make sense since it is a shift from what we currently use. Thus, it would also lead to some adjustment time 👍 That being said, I would just like to point out a benefit of using repositories I forgot the first time. On flows such as these:
having a central point where information is fetched and observed from, ensures the UI is currently reflecting the most recent updates. Additionally, if the data (subscriptions in this case) is updated in a centralized place, there is no need to re-fetch information again when entering or re-entering screens that need it - resulting also in less busy instances (not sure about this part, please current me if I'm wrong). That being said, I agree that it may not make sense such changes 👍 |
Piped uses ActiveJ (web framework) supporting caching, and hence it detects that the request has already been made and thus cached before -> it will immediately return the cached response without that the server is doing any work to refresh the data. After some time, the cached response will be removed from the cache, however when it is it's been enough time passed so that it really makes sense to refresh the data due to possible updates. So the impact mostly should be minimal (as far as I know). But it might however improve the UX to not needing to wait for the cached response to be returned again. |
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!
Feature - Improve UX for new users
Some users have the bad luck of not picking a working instance on the first try. This development aims to offer them support and guide them into selecting a working instance.
Types of instances
For new users, instances can be divided into three types.
How can we improve the UX to facilitate the selection of an appropriate instance?
Depending on the type of instance the user has selected, the way the user will be guided will be different.
For users with an instance of:
Adjacent issues
While working on this feature, there were a couple of behaviours that could be improved:
binding
being null when it was time to fill the views) - sample below ("current_ui_behaviour.mp4");current_ui_behaviour.mp4
before_tab_swap.mp4
What does this PR do?
HomeViewModel
responsible for the calls that fill the screen and is responsible for holding the LiveData instances that have the data being shown - Subscribing (on onResume) and Unsubscribing (on onPause) to these LiveData instances leads to a healthy UI/IO management, and supports data being loaded, even if we are not in the home screen;Notes
Showcase
showcase.mp4
Showcase of the current approach cancelling previous, ongoing threads when creating new ones - video sample.
after_ui_behaviour.mp4
after_tab_swap.mp4
Test cases
This was the main test case I used to test this development. Please let me know if there are other "must test" flows.
3a. Ensure the progress bar briefly shows up, indicating ongoing attempt;
4a. Ensure redirection to Settings > Instance;
6a. Ensure the progress bar is enabled, indicating an ongoing attempt;
8a. Ensure the SnackBar DOES NOT shows up;
10a. Ensure the SnackBar DOES show up;
13a. Ensure the Home screen is correctly loaded;
Closing Notes
I sincerely appreciate the review; I hope this improves both new users' UX and the home load.
Please let me know if any of the points or approaches mentioned do not make sense or if we should opt for another approach.