Skip to content

Finish view model for contact list

Mikolai Gütschow requested to merge 92-contact-list-viewmodel into main

As a followup of !30 (merged), this MR moves all application logic related to the contact list out of the composable UI into a view model. This view model updates its state when receiving events (e.g. contact added/removed/connected/disconnected) and the new state gets automatically reflected in the UI. I've taken inspiration from the briar-android code, especially the contact package for the implementation of the view model.

The complete diff turned out to be quite long, so here a quick summary on what I did:

  • (re-)add a ContactItem data class holding the information needed for the contact list (apart from the Contact itself: the connection status, the number of unread messages and a timestamp of the last message. Inspired by https://code.briarproject.org/briar/briar/-/blob/master/briar-android/src/main/java/org/briarproject/briar/android/contact/ContactItem.java
  • ContactsViewModel is an abstract class providing common functionality for ContactListViewModel and IntroductionViewModel (similar to Android)
  • maintaining the search value and filtering of the contact list is done inside the ContactListViewModel
  • move introduction related code to the introduction package to match the Android codebase
  • move the logic for the add contact dialog to the AddContactViewModel
  • move remember { mutableStateOf(...) } (state still residing in the UI) as much "down" as possible to prevent passing too many parameters around
  • factor the UiPlaceholder composable out that can be reused at all places where the actual UI is still to be implemented

Several things are not optimal in the current implementation, and should be addressed in the future:

  • All view models are injected into the BriarUi class (the entrypoint of the UI) and passed down all the way to the composables that actually need it. This also means that all view models are instantiated at application startup even if they are not needed from the beginning. Also, view models are used as singletons, i.e., the AddContact or Introduction dialog share the same view model when opened repeatedly. I'm not fully sure how this is handled in Android, but I think view models are tied to the lifecycle of the activity and as the add contact functionality is a separate activity (?) in the Android app, view models would not be reused there. Perhaps we actually want to have the notion of lifecycles at some point also in our Desktop app? Another downside of having view models created already on startup and never destroyed is the fact that they will listen for events all the time even when they are not used at all.
  • All interactions with the Briar API still happen on the main thread. The Android app uses the runOnDbThread callback everytime when accessing Briar APIs. I'm pretty sure we should do a similar thing, perhaps using Kotlin coroutines? Also, we should double-check that is is actually safe to listen to events on the main thread, but that's what I saw in the Android app as well.
  • The State class provided by Compose is actually not as useful as the Android LiveData. For example, it lacks equivalents to the Transformations.map and Transformations.switchMap functions, that would allow for shorter code in the ContactListViewModel when updating the filter on the contact list. Basically, states are not "observable" as LiveData is, but they are nothing more than a container for data. I think we should reconsider using Kotlin coroutines' StateFlow or something similar instead of simple States in the view models.
  • I'm not sure whether the interconnection between different view models is already optimal. For now, the UI (Composables) at some point ties them together. For example, PrivateMessageView uses the Conversation depending on the selectedContact in the contactListViewModel. When implementing the conversation view model (for the private chat), the information on which contact is selected must somehow be passed to the view model, preferably only once when it actually changes. I'm not sure how that would look like, to be honest.

Related to #90 (closed) and #68 (closed), closes #92 (closed).

Merge request reports