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

ViewModels (a start) #5482

Merged
merged 21 commits into from
Mar 8, 2024
Merged

ViewModels (a start) #5482

merged 21 commits into from
Mar 8, 2024

Conversation

westnordost
Copy link
Member

@westnordost westnordost commented Feb 11, 2024

A start for #5070

So, ultimately we want to end up having a ViewModel for every Fragment that isn't purely UI. Until the switch to Jetpack Compose is complete, we can limit that to those places where it is worthwhile to switch, i.e. Fragments that have a lot of state (and would otherwise need to re-fetch this state from database or save it otherwise on onDestroy).

Now, this draft PR is just the first foray into that. Finding the patterns that fit best and can be applied consistently throughout the app and are convenient to use. It is draft because it is open for comments. For this, I tried to create two ViewModels for two Fragments, as an example.

Comments welcome

Our mid term goal is to migrate the UI also to Jetpack Compose, so we should keep that in mind to not necessitate another migration later. Also, the data layer in StreetComplete is blocking, i.e. nothing with coroutines or StateFlow. Hence, in the current architecture, it is up to the ViewModels to wrap everything up into launch et cetera.

ViewModels crash course

ViewModels hold and manage view state of a fragment/activity. They make it easier to hold the UI state because they transcend the view lifecycle. Furthermore, they manage access to the data layer, so the only thing that remains in fragments is controlling the UI.
ViewModels usually have a range of properties wrapped in StateFlows, these are basically (coroutine friendly) observable properties, i.e. the fragment can do something if any StateFlow changes.

@westnordost
Copy link
Member Author

The main part is

For the logs, I also made a data model immutable and a few other minor things, but this is not really too relevant on the ViewModel stuff.

Copy link
Contributor

@Doomsdayrs Doomsdayrs left a comment

Choose a reason for hiding this comment

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

  • Comments left on what I noticed.
  • Suggestible to ensure View Models & Models are in their own directory to keep things organized and easy to locate.

@westnordost
Copy link
Member Author

westnordost commented Feb 12, 2024

Regarding the comments on view logic in the UI code (rather than ViewModel):

Well, currently I envisioned ViewModels to have the following responsibilities:

  • hold UI state to transcend UI lifecycle (as a matter of course)
  • manage access to data layer
  • (UI) business logic

An example of what I understand as (a state in) UI business logic would be

val isLoggedIn: Boolean

while an example of what I understand as (a state in) pure UI logic would be

val isLogoutButtonVisible: Boolean
//or
val rankViewBackgroundTint: Color

Now, there is obviously a smooth transition between the two. So far, I pretty much left out anything from the ViewModel that would be considered pure view logic.
Are there any arguments for a certain middle ground or one or the other extreme?

One argument for pulling as much as possible into the ViewModel would be if you want to have the platform specific UI code as small as possible, e.g. when you reimplement the UI natively on iOS and Android, but we'll not do that, we will use Compose Multiplatform, so the UI does not need to be written 2 times.

@westnordost
Copy link
Member Author

westnordost commented Feb 12, 2024

FWIW in Android architecture samples using Android views, the view model defines state even down to which drawables to use:

https://github.com/android/architecture-samples/blob/views/app/src/main/java/com/example/android/architecture/blueprints/todoapp/tasks/TasksViewModel.kt#L73C9-L73C22

On the other hand, the same ViewModel in the same architecture sample when using Jetpack Compose as UI framework looks completely different. No defining which drawables to use at all, and using the "UIState" pattern:

https://github.com/android/architecture-samples/blob/main/app/src/main/java/com/example/android/architecture/blueprints/todoapp/tasks/TasksViewModel.kt#L73C9-L73C22

I take away from that there doesn't exist one best practice for ViewModels but a lot of variation is possible depending on the project. I also take away from that when migrating to Compose, I can probably kiss goodbye to idea of not touching the ViewModels again to achieve best readability / best practices. Which was probably an unrealistic hope, given that it is recommended to have one ViewModel per screen in the end with Compose, not one ViewModel per Fragment

Which also means that I will want to keep the ViewModels relatively bare bone for now to not duplicate effort when having to touch them again later.

@Doomsdayrs
Copy link
Contributor

I am going to make a branch with a few commits, as I feel like that would be faster to explain what I notice.

@Doomsdayrs
Copy link
Contributor

The more I look at this code the more horrified I am by the duplicated code and its efforts.

For example, the Dao is not an actual Room DAO, but a hand crafted SQL wrapper. Room already handles generation of this code, but it also provides functionality to directly observe the database changes.
https://medium.com/androiddevelopers/room-flow-273acffe5b57

This then leads to using LogController to create its own listener system to handle feedback.

Which then leads to horrific memory duplication in the UI layer.

@westnordost
Copy link
Member Author

westnordost commented Feb 12, 2024

Yes, the app doesn't use Room, it is older than Room, modern architecture (Jetpack) components etc.. (development started 9 years ago. Originally it was written in pure Java. We've come a long way since then)

It is also not a possibility to migrate to Room because it is not multiplatform. (Subsequently) the data layer is also different from what one finds in modern guides. I think I noted that here or in the other issue ticket yesterday.

It is not out of question to refactor this one day to be more in line with modern architecture design guides, but it is important to do such refactorings one step at a time and then each consistently finishing that refactoring step before continuing onto the next one, otherwise one will end up with an unmaintainable mess of inconsistently applied patterns. For now, what's on the menu is to consistently introduce ViewModels (- and after that or partly intermixed with that use Jetpack Compose for UI.) - this means that it falls to the ViewModels to stream the data into flows because the data layer is not already flow/coroutine-based.

@westnordost
Copy link
Member Author

westnordost commented Feb 12, 2024

(In case you are interested, here is a bird's eye overview over the data layer only of the app, it also includes explanations about the responsibilities of the different class types: https://raw.githubusercontent.com/streetcomplete/StreetComplete/master/res/documentation/overview_data_flow.svg . As you see, it is quite extensive, and actually, the Logs stuff is missing from this, because it is pretty new)

Doomsdayrs added a commit to Doomsdayrs/StreetComplete that referenced this pull request Feb 14, 2024
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
@Doomsdayrs
Copy link
Contributor

It took me a bit to test due to the snow storm here in New York.

The only think I wonder is how to get a log to occur while in the logs view.

How does 1cce1a0 look to you @westnordost ?

@westnordost
Copy link
Member Author

Oh yeah, I see its circulating in the news right now. In Hamburg, on the other hand, it's just mud season (40°F, heavy clouds, bit of rain).

Easiest way to make logs appear while being in the log view is to start a download (menu -> download map here) and then go to the logs view. The download usually takes a bit (depending on the download map size and data density) and logs a lot of things. (Since the log view is rarely opened and things are rarely logged while in that view, it would actually not be that bad if updating the live logs was hugely inefficient)

Ah, yes, I remember I've looked at the documentation of callback flow (+ transformLatest, stateIn) yesterday and was unable to wrap my head around it. Maybe I need to need to look through the flow documentation in general again first I'll try again tomorrow.

Doomsdayrs added a commit to Doomsdayrs/StreetComplete that referenced this pull request Feb 15, 2024
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
Doomsdayrs added a commit to Doomsdayrs/StreetComplete that referenced this pull request Feb 15, 2024
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
@Doomsdayrs
Copy link
Contributor

Oh yeah, I see its circulating in the news right now. In Hamburg, on the other hand, it's just mud season (40°F, heavy clouds, bit of rain).

The weather for the Winter was so far Muddy at a consistent 10°C~.

Easiest way to make logs appear while being in the log view is to start a download (menu -> download map here) and then go to the logs view. The download usually takes a bit (depending on the download map size and data density) and logs a lot of things. (Since the log view is rarely opened and things are rarely logged while in that view, it would actually not be that bad if updating the live logs was hugely inefficient)

Corrected a small issue that I remembered, but did not remember at the time of writing with 0ffddeb.

Also optimized the view while I was at it with b2e90ee.

Ah, yes, I remember I've looked at the documentation of callback flow (+ transformLatest, stateIn) yesterday and was unable to wrap my head around it. Maybe I need to need to look through the flow documentation in general again first I'll try again tomorrow.

If you would like, we can skill share via a voice call & screen share? I should be free after 15:00 EST.

@westnordost
Copy link
Member Author

westnordost commented Feb 15, 2024

Alright, I am back, I have collected all the information about flows from the docs, they are indeed quite cool. Though, state flows are even hotter. Anyway... 🙄

Having understood what the code does, it does really look quite sleek and/or clever, although I wouldn't have been able to write that. Maybe at a later time, with more experience.

However, it's nice and all that this is idiomatic (experimental?) Kotlin flow-code, but doesn't 0ffddeb make it hugely inefficient again, as the whole list is copied for every single log entry added again?

Also optimized the view while I was at it with b2e90ee.

Uff, to use your words, it's horrible to see how this is getting more and more complicated. The more complicated the code, the more difficult to maintain. In prospect of the UI to be reimplemented in Compose, I'd also have not bothered with implementing that, but I am fine with merging your improvement. It is good that in very few places in the app, there is such a live-stream of new data that should be displayed.

Anyway, feel free to create a PR that merges into this PR, it is maybe easier to talk about the improvements then.

Regarding the video call, rather not for now, flows are very new to me and I am slow on the uptake to understand new concepts, I don't think a quick explanation would help.

@Doomsdayrs
Copy link
Contributor

However, it's nice and all that this is idiomatic (experimental?)

They mark it experimental, it hasn't changed for years now. It's just a liability thing for themselves.

Kotlin flow-code, but doesn't 0ffddeb make it hugely inefficient again, as the whole list is copied for every single log entry added again?

Yes, which is annoying. But it happens in a flow now, and should not block the UI. And the JVM will optimize the routine after the first 2~ iterations. From testing, I saw no visible lag. Do you see any lag?

Aside the greater lag came from using notifyDataSetChanged on the UI thread 😉 .

Uff, to use your words, it's horrible to see how this is getting more and more complicated. The more complicated the code, the more difficult to maintain.

We currently face a code base that was built without proper architecture & abstractions. Technical debt is a debt, one has to pay eventually.

In prospect of the UI to be reimplemented in Compose, I'd also have not bothered with implementing that,

Mix implementation is possible, but do note the application will bloat in size slightly holding two UI stacks.

Keep things as organized as you have, slow and steady implementation can be achieved.

but I am fine with merging your improvement. It is good that in very few places in the app, there is such a live-stream of new data that should be displayed.

I can later spend some time quickly making a PR that implements DiffUtil throughout the application.

A separate effort should be made to replace the current fake "Daos" with real Room Daos, since that would help with proper live-streams from the DB without the duplication that currently occurs.

Anyway, feel free to create a PR that merges into this PR, it is maybe easier to talk about the improvements then.

Alright, will do that soon.

Regarding the video call, rather not for now, flows are very new to me and I am slow on the uptake to understand new concepts, I don't think a quick explanation would help.

Not a problem, Just offering a sturdier hand if needed.

@westnordost
Copy link
Member Author

westnordost commented Feb 15, 2024

Hmm, well, thank you for the review and all those suggestions. This PR is a sort of test or model how the viewmodels should look after the refactor. I think I will roughly follow these guidelines:

  • no UI (framework) specific code in ViewModels, no Android-specific stuff in the ViewModels (it is TBD where that code should go, for the time being, it stays in the Fragments)
  • only use StateFlows when it is actually needed, i.e. values that are displayed and expected to change, otherwise just normal fields to keep complexity down. For data that is fetched asynchronously (from DB), there doesn't seem to be another choice, though.
  • bundling observable data into a data structure as done in Android examples is okay for related data (from same data source). If it leads to more readable and/or less code and is not performance critical, bundling it all into an ui state data class would be fine, too. Compose does not really care anyway.
  • have an accompanying interface for the ViewModel in order to not expose MutableStateFlow on its interface (but just the StateFlow)
  • prefer not exposing MutableStateFlow on ViewModel interface (use setters instead), for cleaner interface
  • keep directly view related data in the view rather than in the view model (isLoggedIn in in the viewmodel, something like rankBackgroundGradient in the view). May change later during refactor to Jetpack Compose.

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:
#5482
Doomsdayrs and others added 5 commits February 15, 2024 20:29
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:
#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:
#5482
@Doomsdayrs
Copy link
Contributor

Isn't dc7768f more complicated then 5dcc124 ?

@westnordost
Copy link
Member Author

westnordost commented Feb 20, 2024

CopyOnWriteArrayList is JVM-only. Plus, it does not solve the performance issue, because... on every write, the array is still copied, just like the name says :-)

Regarding replayCache[0], I find it relatively ugly to have that on the interface. From the point of view of any observer, it is an implementation detail that internally, logs is actually a SharedFlow.

@westnordost westnordost marked this pull request as ready for review March 8, 2024 22:47
@westnordost westnordost merged commit ace1fbd into master Mar 8, 2024
@FloEdelmann FloEdelmann deleted the viewmodels branch March 8, 2024 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Released
Development

Successfully merging this pull request may close these issues.

2 participants