ViewModels (a start)

[westnordost]

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]

The main part is

• ProfileFragment.kt and ProfileViewModelImpl.kt
• LogsFragment.kt and LogsViewModelImpl.kt

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.

[westnordost]

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]

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]

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]

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]

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]

(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]

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]

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]

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 0ffddeb5e557bc560038c93e788cdfe8fa021c6f.

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

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]

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 https://github.com/streetcomplete/StreetComplete/commit/0ffddeb5e557bc560038c93e788cdfe8fa021c6f 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 https://github.com/streetcomplete/StreetComplete/commit/b2e90eec7affc84c25df383e0a71d0df442f3f9c.

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]

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 :wink: .

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]

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.

[Doomsdayrs]

Isn't dc7768f more complicated then 5dcc124 ?

[westnordost]

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.