-
-
Notifications
You must be signed in to change notification settings - Fork 359
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
Use ViewModels #5070
Comments
While it is nearing the end of the summer for me. I have experience with implementing view models into android apps. Would you care for some help in regards to this? |
Absolutely! However, I am currently researching into Jetpack Compose / Compose Multiplatform and I didn't find out yet whether this'd use a different solution altogether. Do you know anything about this? Have you used Jetpack Compose before? |
@westnordost Jetpack Compose can absolutely be used with ViewModels! In the case of a pure Compose architecture, various navigation solutions allow you to have proper scoping. However, a complete Compose rewrite of StreetComplete feels like a pipe dream, a branch that'll just live on the side and hopefully, when it's complete, it releases and more bugs have to be resolved. Not to mention some fun things, like map integrations. Having access to a MapView in a Compose-friendly manner requires _lots_of work (see https://github.com/googlemaps/android-maps-compose, where you need quite a bit of Compse-specific knowledge to make an implementation of it. It could be accessed with an AndroidView, but even then, that's quite a bit of work. A happy middle ground that can slowly be released and tested out would be keeping Fragments, moving all the state into ViewModels, and using Compose for the UI (returning a ComposeView in onCreateView). Funnily enough, I saw this issue as I checked the repo wondering if some help with Compose or code cleanups would be needed, perfect timing :D As for Compose Multiplatform, I don't believe it'd be of much use for now. Ignoring the fact that I don't believe there is StreetComplete for iOS, the implementation is a bit dodgy at the moment. Also, many composables would be missing for other platforms like desktop, etc. EDIT: if you'd be interested, I could do a quick rewrite of a fragment using both a ViewModel and Compose, to see the difference. |
That's good to hear! I have neither worked with view models in depth yet nor with compose and wasn't sure whether compose would already take care of reinstantiating the state after the view is recreated in some way or another ( I am only contemplating about a compose rewrite in context of compose multiplatform for a possible iOS version (see my last comment). In what way do you think the implementation is dodgy? In the sense that it is still alpha, or something else? (I thought the API would be mostly 1:1 the same as Jetpack compose, as it is mostly just an extension to it.) I agree that using compose just because it might be more modern or convenient is absolutely not worth it, as the undertaking would be huge while there is pretty much no gain, the current UI implementation already works, after all. As with any huge undertaking, there is also the high risk of it never seeing the light of day anyway. Anyway, yes, I'd be interested to see how a fragment would look like with a view model and compose! If not for anything else, for learning purposes. However, back to the topic at hand, it looks like view models could be implemented now! First, to replace the awkward |
As expected, the moment I get to work again on an open source project, work comes back with a vengeance and I end up disappearing. Sorry :D Yes, I meant dodgy as in, the implementation, especially of default material components isn't quite there yet (but getting better!). In addition, KMP can only do interop with Objective-C API, which are notably much less pleasant than the Swift ones to work with. It's serviceable, but not the best experience. (and of course, you still need a mac anyways to work on it and test it, but that's to be expected out of the iOS ecosystem)
Yep! If you're looking to instantiate an android view in a Composable, @composable AndroidView() exists, and you can render a composable into a normal view with ComposeView. To give you an idea, I rewrote the Note creation view in Compose, making the fragment simply return a ComposeView, in a matter of few hours, along with removing the use of some 9 patches. So, it's relatively easy to get something that works everywhere while making things a little bit easier for yourself.
Yep! I haven't looked too deep into the code yet, but I believe the map is just a fragment, on which other fragments get overlaid ? In which case, it can absolutely stay this way. Actually, unless you have a very, very good reason to go full compose, keeping the interop with Fragments/UIViewControllers is a very good option.
I'll try to clean up what I have and make a pull request (either here, or on another repo) in a week or so! |
No worries.
It's a view, in tangram-es at least.
Well I figure a very good reason would be to make the UI fully multiplatform(?). Anything that is a fragment would still need to be reimplemented (and maintained) on the iOS side with UIViewControllers.
Cool, great! However, for now, this wouldn't be merged and would be for educational purposes. A PR that just uses a viewmodel on the other hand could be merged. |
Actually, it could very likely be merged even when using view models + jetpack compose. More about that later, in any case I would be very interested in reading the code of your port (of a fragment) to viewmodel (+jetpack compose), as I am new to both. |
(By the way, a decision has been made to convert the UI to Compose and use ViewModels) Anyway, regarding this ticket, consider this a master ticket: PRs can be made to add ViewModels to single Fragments to advance this ticket, it is neither necessary nor wise to do this all in one big PR, covering the whole app at once. |
I started experimenting with it. There are a few choices one has to make in regards to how the ViewModels should be structured, which patterns to use. One of these is whether view models should generally look like this: class MyViewModel : ViewModel() {
private val _a = MutableStateFlow<String?>(null)
val a: StateFlow<String?> get() = _a
private val _b = MutableStateFlow<String?>(null)
val b: StateFlow<String?> get() = _b
// and then updates e.g.
fun updateA() {
_a.value = "foo"
}
} or like this data class MyUiState(
val a: String? = null,
val b: String? = null
)
class MyViewModel : ViewModel() {
private val _state = MutableStateFlow<MyUiState>(MyUiState())
val state: StateFlow<MyUiState> get() = _state
// and then updates e.g.
fun updateA() {
_state.update { it.copy(a = "foo") }
}
} The difference is that in the first, properties are updated and observed for changes each separately. The second keeps all state in one immutable data class, on each update of a single property, the state is copied to include the change of the single property. Changes are (thus) observed in a non-granular way, i.e. all the UI is updated even when a single property is changed. The second method sound hugely wasteful in terms of performance, however:
So, I tend towards using the second method, but it will be less performant (until we have switched to Compose, or always). What do you think? Will post some actual code in a draft PR soon. |
Abstract with an interace so you don't need to do the private vs public mess interface VM{
val flow: StateFlow<T>
}
class ImplVM : VM {
// mutable state flow for states
// errors should be pushed to UI using a mutable shared flow
override val flow : MutableStateFlow<T> = MutableStateFlow<T>(emptyValue)
} I recently converted my entire application (Shosetsu) (Pending new Release) into compose, because I had already established flowful VMs my migration was smooth as butter. Do note that flowful VMs also provide the benefit of being able to quickly stream and manipulate data. For example the UI is a list with a state flow backing it holding the list. There are filters (another stateflow) dependent on what's in the list as well. Example in my application. |
Hmm, the answer is orthogonal to my question, but nonetheless useful, thank you! Right, good hint with the interface. Although, of course, this doesn't save any lines of code. Also, you didn't seem to have done that in the ViewModel you linked to as an example, though? You mean with flowful VMs that they use |
Apologies, I saw your message and had to get it out of my mind as soon as possible (while on transit). I should be able to make a better response when I am more settled down, which should be now.
D'oh, I had thought that would have been the best example (For using overrides), since I saw it had my mutable shared flow mention in it. This is my SearchViewModel and it's abstraction ASearchViewModel. It does utilize the interface method I referred to prior.
Apologies, it seems I had totally misread what you sent (due to email shenanigans), and had assumed you were putting compose state holders in the VM ( that I have seen done before and was traumatized by) . A better response is as follows.
While copying is cheap, one must thing about how the UI will look, essentially "stuttering" in between states if we assume that there would be a loading frame. Let's do an example in practice with mastodon Read the following instructions completely before performing them.
let's view this in a Kotlin manner. class VM {
val serverInfo : StateFlow<A>
val content: StateFlow<List<B>> // maybe a paging data but that is another topic
val rightColumn : StateFlow<C>
val tabs : StateFlow<List<D>>
val title: StateFlow<String>
} Each of the state flows is loading separately, serverInfo, rightColumn, content, tabs, title etc. Now how long each takes varies yes, but the user already sees enough of the page and would be satisfied, knowing the content is just a moment away. Imagine this if we followed this "pattern" that android uses, in which the ENTIRE UI is a single Android uses it likely as a means of simplifying what is going on. But in writing performant UIs, you want to minimize the time it takes for the general shape of the UI to load in, ... In Shosetsu, the most clunky UIs are NovelInfo, LibraryView, & ChapteReader. Let's take the NovelInfo for example, with its NovelViewModel and its abstraction ANovelViewModel Some Novels have upwards of 3 thousand chapters, which can lead to seconds of wait even on fast devices. abstract val novelLive: StateFlow<NovelUI?>
abstract val chaptersLive: StateFlow<ImmutableList<ChapterUI>> ... One also needs to apply a semi case by case basis in how abstract val selectedChaptersState: StateFlow<SelectedChaptersState> Take example this from /**
* @param showRemoveBookmark If any chapters are bookmarked, show the remove bookmark logo
* @param showBookmark If any chapters are not bookmarked, show bookmark
* @param showDelete If any are downloaded, show delete
* @param showDownload If any are not downloaded, show download option
* @param showMarkAsRead If any are unread, show read option
* @param showMarkAsUnread If any are read, show unread option
*/
@Immutable
data class SelectedChaptersState(
val showRemoveBookmark: Boolean = false,
val showBookmark: Boolean = false,
val showDelete: Boolean = false,
val showDownload: Boolean = false,
val showMarkAsRead: Boolean = false,
val showMarkAsUnread: Boolean = false
) The reason for this, is that the "SelectedChaptersState" is a ton of related data (all appearing on the same bar in app) that wouldn't make sense to have in 6 different flows. So Coalescing data together into a |
Right, I followed your hint regarding the interfaces (though they have to be abstract classes for now, hoping that can be changed when switching from Android ViewModels to a multiplatform implementation) and also made all those observable state flows quite granular. (It's much less of a pain when there is not that mess with the I notice you follow a lot of best practices otherwise with the app. I need to wire the ViewModels to the data layer of StreetComplete, which is structured differently that what one can read in modern guides. (Very short crash course: I've posted the draft PR for the first ViewModels in #5482 in case you want to have a look. (I'd like that.) |
// commom
expect class Foo {
fun bar()
}
// android
actual class Foo : ViewModel {
Fun bar()
}
// ios
actual class Foo : IOSViewModel {
Fun bar()
}
|
We plan to use some framework that supplies multiplatform view models, see #5421 (comment) . In one of the choices, view models were actually just interfaces. |
I'll close this issue and create a new master ticket for using viewmodels with summarized information: #5530 |
So far, the data in the view layer has been kept in the
Fragment
s andActivity
s. Since these can be destroyed and recreated when the Android system decides to do this, the serialization and deserialization into asavedInstanceState
must be done manually. Moving to ViewModels has the following advantages:It's less error prone, possibly also less code. The data is not bound to the view lifecycle and hence does not have to be manually re-created
It provides a better separation between view logic and view data. Thus, the latter is likely mostly platform independent and could be re-used on other platforms with less effort. (View model patterns exists for iOS too, there is at least one kotlin multiplatform library in development that one might switch to in the future).
It will likely simplify some of the "talks to parent via listener" pattern, because some, or most of it(?) can probably be done with shared view models instead (i.e. a view model that is shared between a fragment and its parent fragment / activity).
It's what the cool kids use! Joking. It is actually pretty standard to use since many years and the more standard things are used in StreetComplete, the easier it is for new contributors to join
The text was updated successfully, but these errors were encountered: