architecture-components-samples icon indicating copy to clipboard operation
architecture-components-samples copied to clipboard

View actions (snackbar, activity navigation, ...) in ViewModel

Open fabioCollini opened this issue 7 years ago • 84 comments

Sometimes the ViewModel needs to invoke an action using an Activity (for example to show a snack bar or to navigate to a new activity). Using MVP it's easy to implement it because the presenter has a reference to the Activity. The ViewModel doesn't contain a reference to the Activity, what's the best way to implement this feature? In the GithubBrowserSample you created a method getErrorMessageIfNotHandled to solve this problem, are there other solutions? In a demo project I am working on I have created a UiActionsLiveData class that allows the ViewModel to invoke an action on the view. The connection between the ViewModel and the view is managed using a LiveData so the ViewModel doesn't contain a reference to the View even if it can invoke actions on it. The usage is simple, the ViewModel can add an action to the UiActionsLiveData:

uiActions.execute { navigationController.showError(it, t.message) }

Thanks to the LiveData implementation the action will be executed on the Activity when it's resumed. I like this solution but it's a bit complicated, an official solution would be great.

fabioCollini avatar Jun 20 '17 19:06 fabioCollini

We created something similar in Blueprints called SingleLiveEvent

https://github.com/googlesamples/android-architecture/blob/dev-todo-mvvm-live/todoapp/app/src/main/java/com/example/android/architecture/blueprints/todoapp/SingleLiveEvent.java

and another version called `SnackbarMessage'

https://github.com/googlesamples/android-architecture/blob/dev-todo-mvvm-live/todoapp/app/src/main/java/com/example/android/architecture/blueprints/todoapp/SnackbarMessage.java

We're thinking whether it should be part of the library.

Thoughts?

Edit: a blog post about events, SingleLiveEvent and a possibly better approach: https://medium.com/google-developers/livedata-with-snackbar-navigation-and-other-events-the-singleliveevent-case-ac2622673150

JoseAlcerreca avatar Jun 21 '17 13:06 JoseAlcerreca

I think using SingleLiveEvent as a basis for other one off (notification style) events works great. I've used this pattern in a similar (internal) framework with an older project. Deploying it as part of the library makes sense, allows us all to not have to reinvent the wheel :smile: .

burntcookie90 avatar Jun 21 '17 13:06 burntcookie90

In my implementation I am saving a list of events but in the SingleLiveEvent class you are managing just a single value. For example if the ViewModel produces more than a value when the Activity is paused the observer will get only the last value produced. Is this an expected behaviour? I would expect to see all the value on the observer (then the View can decide to ignore some values).

fabioCollini avatar Jun 21 '17 19:06 fabioCollini

Interesting, for navigation it makes no sense to hold a list and it's officially not recommended for Snackbar but you can join multiple messages in one. We'll take it into account thanks!

JoseAlcerreca avatar Jun 22 '17 13:06 JoseAlcerreca

A big NO from my side for SingleLiveEvent! Unidirectional Dataflow and immutability ftw! (also a little state reducer over here and there doesn't hurt)

SearchViewModel which offers a LiveData<LoadMoreState> getLoadMoreStatus() method for the view to subscribe / listen to. So once the view has displayed the error message (i.e. with a snackbar) the View calls:

viewModel.setLoadMoreErrorShown();

This will then force the viewModel to emit a new (immutable) LoadMoreState.

class SearchViewModel extends ViewModel {
   private MutableLiveDate<LoadMoreState> loadMoreState;
   ...
   
   // Subscribed by the View
   public LiveData<LoadModeState> getLoadMoreStatus() {
       return loadMoreState;
    }

   public void setLoadMoreErrorShow(){
         loadMoreState.setValue(new LoadMoreState(false, null)); // null means no error message to display , false means don't display loading more progress bar
    }
}

behaves like SingleLiveEvent (i.e. after screen orientation change, snackbar wont show again, because LoadMoreState.errorMessage == null) but is immutable!

sockeqwe avatar Jun 22 '17 15:06 sockeqwe

I agree that unidirectional dataflow and immutability are great but in this example I have a doubt about your solution: you update a LiveData while you are updating the view because of a modification of the same LiveData. At the beginning of dispatchingValue method there is a check to avoid observer nested invocations, will it work correctly? Are there other cases when this double update of the view can be a problem? For example can you manage more than one field in this way? Or what about multiple updates of the same field when the activity is paused, will the state contain a list instead of a single value? However the two solutions are quite similar, in my opinion a SingleLiveEvent is not a violation of immutability and unidirectional dataflow. In your example the view invokes a method on the ViewModel to update the LiveData, using a SingleLiveEvent the LiveData is updated in a similar way automatically.

fabioCollini avatar Jun 23 '17 07:06 fabioCollini

At the beginning of dispatchingValue method there is a check to avoid observer nested invocations

Well, I wasn't aware of this implementation detail. I was just saying that to me invoking something like setLoadMoreErrorShown() seems more correct, transparent and easier to test compared to SingleLiveData. I'm wondering if one could implement it's own LiveData class without this check (or at least allow it if the value to dispatch has changed)

For example can you manage more than one field in this way?

I cant see why not.

Or what about multiple updates of the same field when the activity is paused, will the state contain a list instead of a single value?

No, just the latest value, not a list. In contrast to MVP, in MVVM ViewModels typically have this fields to observe a certain value. I cant find a use case where I need to.keep track of previous undispatched values. Do you?

in my opinion a SingleLiveEvent is not a violation of immutability and unidirectional dataflow.

I disagree, it clearly is a side effect (not a pure function) and obviously not immutable and not a unidirectional dataflow (state is not changed and maintained from one component). However, if SingleLiveData works for you and then sure go ahead an use it. At the end of the day no user of your app cares about immutability and unidirectional data flow. I was just saying that from my point of View SingleLiveData violates some core principles (like immutability) and that this problem of a snackbar could be solved more idiomatically i.e. viewModel.setLoadMoreErrorShown, but maybe SingleLiveData is a pragmatic solution... practical vs. idealistic ...

sockeqwe avatar Jun 23 '17 08:06 sockeqwe

I didn't read this in too much detail, so sorry if I'm off base.

But the snack bar is a pretty simple solution. Its timed dismissal is just another form of input from your UI

ZakTaccardi avatar Jun 25 '17 20:06 ZakTaccardi

@sockeqwe since your blog post has attracted so much attention, can you confirm the following? It seems that your solution:

  1. requires more logic in the activity (onDismissed listener and a null check)
  2. setLoadMoreErrorShow should be called resetLoadMoreState, or to use the original,setLoadMoreShown, not sure why you added "Error" there.
  3. it behaves differently than SingleLiveData, since you're resetting after dismissal while we have a "fire and forget" approach. I understand you want to store state but there could be a case where the app is opened days after the last use and the user sees a misleading error message from 2 days ago. It might not be a good fit for a Snackbar. It is a better fit for a navigation event, for example.

As already rectified on your blog post, SingleLiveEvent doesn't use null internally. So statements like

state is not changed and maintained from one component

are not true in this case (if I understand you correctly). You thought the activity was setting the event's data to null. Only the ViewModel, IIRC, sets the value. We considered using composition instead of inheritance to protect who sets the value but we would have to proxy things like observeForever().

JoseAlcerreca avatar Jun 26 '17 13:06 JoseAlcerreca

@JoseAlcerreca thanks for your feedback. It's hard to describe this in detail with text only. I have forked the Github client sample and will implement the repo search with what I have in mind. I hope that a concrete code example makes my point more understandable and a better basis for the future discussion, don't you think so? Will work on it on Thursday...

sockeqwe avatar Jun 27 '17 02:06 sockeqwe

At the beginning of dispatchingValue method there is a check to avoid observer nested invocations, will it work correctly?

Yes, it will. This check prevents reentrance in onChanged method of livedata's observers. If an observer in itsonChanged method sets a new value to LiveData it listens to, this new value will be dispatched right after execution of that onChanged method.

svasilinets avatar Jun 27 '17 19:06 svasilinets

I have worked a little bit on refactoring the SearchViewModel of GithubBrowserSample and would like to hear your feedback. My forked repo can be found here: https://github.com/sockeqwe/android-architecture-components

So I have introduced a class SearchState. This class basically represents state and can be "rendered" directly by the view. So instead of LiveData<Resource<List<Repo>>> results and LiveData<LoadMoreState> my refactored SearchViewModel only provides a single LiveData<SearchState> state for the view (SearchFragment) to observe since SearchState contains all state information in one single object.

Please note that you could split this class in something like kotlin's sealed class or other solutions. The code itself is far from perfect in my example. It is just a proof of concept.

Whenever the user triggers an action like typing search, scroll to the end to load next page, this action sinks down to the business logic. The business logic knows the current state and applies this action on it to compute the new state (which is a new immutable state object). For "SingleLiveEvents" like displaying a error message in a SnackBar I would like to suggest to apply the same concept. If SnackBar has shown the message the action "clear load more error message" sinks down to the business logic as it would be any other action like a click on a button.

you update a LiveData while you are updating the view because of a modification of the same LiveData .At the beginning of dispatchingValue method there is a check to avoid observer nested invocations, will it work correctly?

@fabioCollini yes, it does. It works as you can see in my linked example repository.

Regarding @JoseAlcerreca questions:

  1. requires more logic in the activity (onDismissed listener and a null check)

Not sure if I would call a listener "more logic". Then each click listener is also more logic? I guess it depends on how you define logic in context of view (like activity etc.). (btw. the number of lines of code is decreased by about 20 lines, not sure how meaningful this information is though).

  1. setLoadMoreErrorShow should be called resetLoadMoreState, or to use the original,setLoadMoreShown, not sure why you added "Error" there.

Yes, naming is not great.

  1. it behaves differently than SingleLiveData, since you're resetting after dismissal while we have a "fire and forget" approach. I understand you want to store state but there could be a case where the app is opened days after the last use and the user sees a misleading error message from 2 days ago. It might not be a good fit for a Snackbar. It is a better fit for a navigation event, for example.

Yes, this are two different approaches: "fire and forget" vs. "single state driven by the business logic".
Sure, this is more a "religious" question and as you have already noticed I see more value in the later approach (which obviously doesn't mean that this approach is the better one) when it comes to predictable state management, debugging and testing. For example, debugging: We can easily log every step we have made from one state to the other state (and which user interaction was involved to trigger the state transition).

One problem with "fire and forget" is that it may leak View implementation details to the underlying layer (like ViewModel, "business logic", or whatever layer you have in your app). For example: Right now the app is showing a SnackBar if an error has occurred. What If one day we would like to display this error with a traditional TextView? Then we not only have to change code in the view layer, but also in the underlying layers where we have produced the SingleLiveEvent because then it shouldn't be a SingleLiveEvent anymore but for TextView we would use simply MutableLiveData, right?

sockeqwe avatar Jun 30 '17 15:06 sockeqwe

i've implemented similar approach for ui commands https://github.com/deviant-studio/energy-meter-scanner/blob/master/app/src/main/java/ds/meterscanner/mvvm/Extensions.kt#L30

and Snackbar example here https://github.com/deviant-studio/energy-meter-scanner/blob/master/app/src/main/java/ds/meterscanner/mvvm/Command.kt#L8

it would be great to have such functionality out of the box in the upcoming lib release

mykola-dev avatar Jul 04 '17 15:07 mykola-dev

@deviant-studio your Command has two problems:

  • You can't use null as a value
  • If the value is set while an observer is not active, it will be lost. For example, if done from activity.onCreate.

JoseAlcerreca avatar Jul 05 '17 09:07 JoseAlcerreca

@JoseAlcerreca true. that is why i'm going to use your SingleLiveEvent implementation ;)

mykola-dev avatar Jul 05 '17 10:07 mykola-dev

Hi! I just started/learning Android Architecture Components and see that how to handle View Actions is a missing piece on the proposed components. By no means I am criticizing the work done by Google. I love what they have brought to the table.

With that being said, I kind of like @deviant-studio approach to the problem. Making all Commands one shot only. Imagine if this approach can be used together with a Event Queue System that the ViewModel would manage. The Events would be like constant ACTIONS that the Activity/Fragment could subscribe to and would be dispatched in order.

A seudo implementation of this would be: In ViewModel:

dispatch(SHOW_PROGRESS)

//do long work
loadPosts()

dispatch(HIDE_PROGRESS)
dispatch(SHOW_RESULTS)

In Activity:

queueSubscribe {
    switch(event) {
        SHOW_PROGRESS ->  progressBar.setVisibility(View.VISIBLE)
        HIDE_PROGRESS -> progressBar.setVisibility(View.GONE)
        SHOW_RESULTS -> {
            adapter.setPosts(viewModel.posts)
        }
    }
}

Once the event is consumed, it should be dropped from the queue and all should be processed sequentially. In this way, the Activity/Fragment would loose track of all ViewActions that need to be handled by the view.

This is my 2 cents on this. Got the inspiration after working with Vuex (https://vuex.vuejs.org/en/intro.html) on another project (somewhat React-ish) to handle all data flow on one direction.

If this is not good or valid at all, no problem! Just sharing some ideas.

asantibanez avatar Jul 19 '17 03:07 asantibanez

I love the idea of SingleLiveEvent but i'm thinking, theres 10~ish callbacks, is it better to have n unique SingleLiveEvents or would it be better to have a singleliveevent passing in different event types?

gaara87 avatar Jul 21 '17 18:07 gaara87

@JoseAlcerreca We tried your SingleLiveEvent implementation and are having problems when using it together with observeForever. removerObserver isn't working correctly since the original Observer is wrapped inside the SingleLiveEvent. Maybe it needs some kind of internal map of wrapped observers?

I created a naive implementation (in Kotlin) which works for us: https://gist.github.com/etiennewelsch/142ab9e080b0144927fd2486cd34feb9

etiennelenhart avatar Jul 27 '17 07:07 etiennelenhart

@JoseAlcerreca I am successfully using your SingleLiveEvent and vote for including it in the library when released.

jshvarts avatar Aug 03 '17 00:08 jshvarts

@etiennewelsch observeForever is not meant to be used in production (I think!). Even in tests I ended up using observe. If you share more details on how you use it I might be able to help.

JoseAlcerreca avatar Aug 03 '17 08:08 JoseAlcerreca

@gaara87 my suggestion is that you try both approaches, as it doesn't take much time, and decide. In principle 10 callbacks sounds easier to manage than 1.

Edit: But I would like to know your use case, as 10 SingleLiveEvents sounds like a lot!

JoseAlcerreca avatar Aug 03 '17 08:08 JoseAlcerreca

@JoseAlcerreca I was not aware that we shouldn't use observerForever. Good to know, if that's really the case. :) We're currently doing manual Dependency Injection via a custom Application class and are using the SingleLiveEvents to notify AndroidViewModels of events in our business logic. So there is no LifecycleOwner to pass to observe.

etiennelenhart avatar Aug 03 '17 13:08 etiennelenhart

This thing is indeed a interesting discussion. I need a "OneTimeObserver" as well in my project. I ended up in something like that:

    myLiveData.observeForever(object : Observer<String> {
            override fun onChanged(t: String?) {
                mainModelLiveData.value = MainModel(true, t)
                myLiveData.removeObserver(this)
            }
        })

Now I thinking about to use the SingleEvent class. The problem: I use this in a ViewModel. Which means I don't have a LiveCycle here. Which means. The SingleEvent class can't be used.

Why I listen to LiveData in a ViewModel you think? I have a similar architecture like described here: I have a UserRepo which returns a LiveData. But beside of "tunneling" it directly in to the Activtiy/Fragment (within a ViewModel code like that):

fun getUser(): User {
   repo.getUser()
}

I have a similar approve like @sockeqwe. I have one LiveData object which is accessable from the Activtiy/Fragment which sends one Model outside. Like data class Model(var loading: Boolean, var toolbarTitle: String). But my Repo can be load multiple user. Which means I have to listen in my ViewModel to the changes of the LiveData from the Repo. If everything loaded (with the "OneTimeObserver") I can send change the Model object and it will be send to the Activtiy/Fragment...

Yeah a lot of text i know 🙃

But: Are there other solutions? Even if you say observeForever shouldn't be used... Why not? 🤔

StefMa avatar Aug 04 '17 07:08 StefMa

@StefMa of course you can use the LiveData in the ViewModel, just observe it from the activity. You can create a getLiveData method in the VM or an observe(owner, observer). As with any LiveData, the VM exposes it.

JoseAlcerreca avatar Aug 04 '17 12:08 JoseAlcerreca

That's is the point. I don't want to observe it in the activity. As written above. I have multiple LiveData from different "services" in on ViewModel but want only send one "global Modal" to the Activity...

Sent from my Google Nexus 5X using FastHub

StefMa avatar Aug 04 '17 12:08 StefMa

Gotcha. If you're talking with a data source that doesn't need to be scoped to a view, you might want to use observeForever. However, LiveData was designed for the presentation layer, not to replace RX.

JoseAlcerreca avatar Aug 04 '17 13:08 JoseAlcerreca

@JoseAlcerreca @StefMa We came to the same conclusion today and just wrote our own Observable implementation which works equally well without the lifecycle overhead. SingleLiveEvent is still really useful for ViewModel to Activity events though.

etiennelenhart avatar Aug 04 '17 16:08 etiennelenhart

@JoseAlcerreca We came across a possible bug in the SingleLiveEvent implementation. When multiple observers are registered (e.g. two fragments), only the first is notified since pending is reset immediately. Any thoughts?

etiennelenhart avatar Aug 10 '17 14:08 etiennelenhart

Yes, that's working as intended (there's even a warning when you register the second). It's because we can't know how many observers are active (we can only know there are >0) as the LiveData doesn't expose the information.

We created a version that counted the number of active observers but it was too complex for a sample. We'll try to fix it if it's included in the library.

The workaround is to create a SingleLiveEvent per observer.

JoseAlcerreca avatar Aug 10 '17 15:08 JoseAlcerreca

@JoseAlcerreca Ah, sorry I totally forgot that you put in that warning. It seems to be quite difficult to get ViewModel to Activity communication right. I'm pretty confident, that you'll come up with something though. The Components already simplified our projects a lot.

etiennelenhart avatar Aug 11 '17 07:08 etiennelenhart