Skip to content
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

Collect session-list in view-model scope in view models of the favorite list and changes list. #598

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fabianbieler
Copy link

Acknowledgments

Please check the following boxes with an x if they apply:

  • I have read the recent version of the contribution guide before creating this pull request.
  • I have checked a few other pull requests to see how they are written.
  • The feature I want to propose would be useful for the majority of users, not only for me personally.
  • I am aware that EventFahrplan is mostly developed by one person in their unpaid spare time.
  • I can help myself to get this feature implemented or know someone who wants to do it.

Description

For both the change-list and the favorite-list:
collect the Flow of sessions in view-model scope, and send the converted parameters through a channel.

Before

See attached issue

After

Scroll position is preserved.

Resolves #432

...of the favorite list and changes list.

This fixes the bug that the vertical scroll position was not preserved
when navigating to a session-detail-view and back to the list.
@fabianbieler
Copy link
Author

fabianbieler commented Jan 1, 2024

I'll have a look at the failing tests after I got some sleep and update the PR.

Edit: done.

@johnjohndoe johnjohndoe added Bug fix Supposed to be assigned to pull requests. Favorites The favorites screen, starring a session Schedule changes Schedule changes dialog, schedule changes screen UX User experience, accessibility, screen readers, touch feedback, ... Scrolling UI interaction in list and grid views labels Jan 2, 2024
Copy link
Member

@johnjohndoe johnjohndoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue described #432 still happens for me. Tested on a tablet. I updated the description to mention that I reset the system date/time to test the issue.

Feel free to append a recording of your observations to your pull request. Maybe we are doing things differently.

@@ -80,6 +80,7 @@ class ChangeListFragment : AbstractListFragment() {
override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
super.onViewCreated(view, savedInstanceState)
observeViewModel()
viewModel.observeChangeListParameter()
Copy link
Member

@johnjohndoe johnjohndoe Jan 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not call the viewModel directly from a fragment. The view (fragment) is supposed to only listen (observe) to state or event changes published by the view model through its public properties. The view (fragment) has to be "damn stupid".

You can rewrite the view model similar to what is done in FahrplanViewModel. There the init function is used to launch collecting a data stream like your repository.changedSessions.collect.

Same for the StarredListFragment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabianbieler If you have any questions or need help please let me know.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabianbieler Please don't hesitate to reach out if you want assistance here.

@johnjohndoe johnjohndoe added the Stale Inactive or abandoned issue or pull request with no recent activity. label Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug fix Supposed to be assigned to pull requests. Favorites The favorites screen, starring a session Schedule changes Schedule changes dialog, schedule changes screen Scrolling UI interaction in list and grid views Stale Inactive or abandoned issue or pull request with no recent activity. UX User experience, accessibility, screen readers, touch feedback, ...
Development

Successfully merging this pull request may close these issues.

List of favorites scrolls to the top when returning from details screen after delay
2 participants