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

Fixed UI State Loss In BrainzPlayerSearchScreen #534

Conversation

rahul31124
Copy link
Contributor

This PR resolves the issue of UI state loss in BrainzPlayerSearchScreen during screen orientation changes. Previously, the search query and UI state were reset when the screen rotated. This issue has been fixed by introducing state retention in BrainzPlayerViewModel and ensuring that the UI state, including the search query and results, is preserved across orientation changes.

Before

Orientation_Before.mp4

After

Orientation_After.mp4

Copy link
Collaborator

@07jasjeet 07jasjeet left a comment

Choose a reason for hiding this comment

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

Hi @rahul31124, thanks for picking this up. You hit the correct spot by using view-model to preserve state!! But, there are better ways to do this task for which I have left comments in the review itself :)

onQueryChange = { newValue: String ->
val updatedQuery = TextFieldValue(newValue, selection = brainzplayerQueryState.selection)
viewModel.updateSearchQuery(updatedQuery)
viewModel.searchSongs(newValue)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi, we should not call this search function here itself, but in the view-model. We want to debounce this request by about 100-200 millisecs (since its local search, otherwise we prefer 500 when network calls are required). Please take inspiration from the following line:

private val queryFlow = inputQueryFlow.asStateFlow().debounce(500).distinctUntilChanged()

You can go through SearchViewModel.kt to derive your logic for searching.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @07jasjeet ,
I have refactored the BrainzPlayerViewModel and BrainzPlayerSearchScreen as per the guidance,could you please review and let me know if everything looks good?
Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good!

Copy link
Collaborator

@07jasjeet 07jasjeet left a comment

Choose a reason for hiding this comment

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

LGTM, Great work!

@07jasjeet 07jasjeet merged commit feca7ce into metabrainz:main Jan 28, 2025
1 check passed
@07jasjeet
Copy link
Collaborator

@rahul31124 Thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants