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

Implementation ViewModel improvements #5488

Merged
merged 4 commits into from
Feb 15, 2024

Conversation

Doomsdayrs
Copy link
Contributor

From #5482

This paves the way for the next part of my commits.
Previously, the code was not idiomatic to how Kotlin Flows work.
This is corrected with this commit.

To do so, a separate function is used to create a callback flow on the logs controller.
This is then called per new filter, and provided to the UI in that fashion.

In response to work done:
streetcomplete#5482
Because the actual List pointer is the same,
 there is no difference between the value sent to emit and the
 value still in the StateFlow.

To correct this,
 A new list is created by appending an immutable list
 with the new value, which is then saved internally and emitted.

In response to work done:
streetcomplete#5482
LogsFragment was lagging my Android Interop on my System.
This is because `notifyDataSetChanged` is used,
 it is computationally intensive and should never be used.

Implementation of a quick DiffUtil calculator solves this.

In response to work done:
streetcomplete#5482
@westnordost
Copy link
Member

Your suggestion regarding SharedFlow, to solve the array copying, is it (not) also possible here?

@westnordost
Copy link
Member

I mean, it is not that important, as you've tested. It did not lag before and it still does not lag, plus it is a screen that is rarely even visited. Live update of the logs is something of a nice-to-have-feature anyway.

Just curious.

@westnordost westnordost merged commit 04c473c into streetcomplete:viewmodels Feb 15, 2024
@Doomsdayrs
Copy link
Contributor Author

Your suggestion regarding SharedFlow, to solve the array copying, is it (not) also possible here?

I think it was simply time constraints and I forgot.

5dcc124 does this

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