Replace`final` modifier with `@CallSuper` annotation for `MobiusLoopViewModel.onCleared`

[slesinskik]

Would it be possible/feasible to remove final modifier from MobiusLoopViewModel.onCleared and replace it with @CallSuper?

It would preserve the behaviour of the MobiusLoopViewModel class while allowing easier subclassing and clearing of any non-mobius related resources (i.e. rx bus / room subscriptions).

[togi]

I'd recommend putting such resources in your effect handlers and event sources rather than manually managing their lifetimes. The lifecycle of the loop will be the same as the lifecycle of the ViewModel, so using eg. a rx bus or room subscription as an event source will work similarly to if you manage its lifecycle manually in the ViewModel.

We made these methods final on purpose to discourage subclassing in the way you describe, in order to encourage binding your dependencies to the lifecycle of the loop. In fact at first we wanted to make it so that MobiusLoopViewModel was a final class, but that ended up not being very practical. Of course if you have a use-case that isn't well supported we could revisit the decision, or see if there is some other way we could help you achieve what you're trying to do. Could you describe a bit more why you want to manage it manually?

[slesinskik]

I've been using eventSource quite successfully so far and it's been playing very nicely with ViewModel's lifecycle - especially for events that are emitted sporadically and can be processed quickly (i.e. events that don't emit any expensive, or long-running, effects). Or when used with a loop bound to a LifecycleOwner (i.e. activity or a fragment) :)

There are actually two issues I am trying to address :) The first one is that Mobius loop is active for as long as ViewModel is alive - which might include long periods of Activity (or a Fragment) being stopped / in the background. Binding loop to ViewModel is quite useful, because it allows us to survive configuration changes (i.e. screen rotation) without interrupting loop's work. However, with multiple stopped Activities in a task, all with active loops (i.e. observing db, rxbus, or network state changes), we might end up with quite a lot of (unnecessary) background processing. It seems like it would be very beneficial (from performance / battery point of view) to be able to limit processing of certain events to when MobiusLoopViewModel.modelData is active (having at least 1 active observer). For that reason, some of our current "event sources" are managed outside of eventSource().

Example: Activity/loop observes db changes (event), performs some long running effect (queries network api / processes data), updates model. It is perfectly fine if such events are handled when activity/fragment with a loop is active, but not so much when we have a few stopped activities like that in a task stack and the db is modified by a WorkManager, or a currently active loop.

I can think of at least 2 solutions to prevent such background processing:

• Propagating ViewModel state (preferably active/inactive state of modelData) into our Mobius model and handling events appropriately (i.e. ignoring them / storing for future processing / etc) - this is doable, but requires quite a bit of extra code and complicates update method - it also doesn't prevent the cost of keeping such observable alive. On top of that, afaik, the easiest and safest way to observe active/inactive state is to hook into onActive/onInactive of modelData, i.e. by wrapping it in a custom MediatorLiveData (and overriding getModels, currently final, to return the wrapper).

• Preventing those events from being observed (or emitted) at all when modelData becomes inactive - which also requires some form of LiveData.onActive/onInactive callback.

I can always subclass MobiusLoopViewModel, add my own custom LiveData with onActive/onInactive callbacks and manage observers, or EventSources, there. But that would require all activities/fragments to observe not only MobiusLoopViewModel.modelData but my custom, dummy live data as well. Not the prettiest solution, and a bit error prone, but doable. A bit prettier solution would be to override modelData, wrapping it in my custom MediatorLiveData that exposes onActive/onInactive callbacks. Or modify MobiusLoopViewModel to expose appropriate methods :)

If you have a better solution for the above issue, I'd be very grateful for suggestions :)

The second issue I have is that I have to deal with quite a lot of legacy code that lives in currently existing ViewModels that extensively make use of ViewModel.onCleared. It would have been a bit easier if I could simply add Mobius loop to the existing ViewModels (and integrate the legacy code using dispatchEvent method), rather than keeping two separate ViewModels (the legacy one and the Mobius-only one) and wiring them up together somehow at the Activity/Fragment level.

[togi]

Hmm, one idea that comes to mind is to bake your first solution into the framework itself, and expose a LiveData<ModelDataState> or something like that, in a similar way that we currently pass you a Consumer<V> for view effects. This would mean your effects and event sources would be able to observe that active/inactive state of modelData directly if they want to enable/disable themselves. And if for some reason you'd rather handle this in the update function, you can do that too by turning it into an event source.

In practice that would basically mean changing the loopFactoryProvider from being Function<Consumer<V>, MobiusLoop.Factory<M, E, F>> to something more like this:

public interface LoopFactoryProvider<M, E, F, V> {
  MobiusLoop.Factory<M, E, F> get(Consumer<V> viewEffects, LiveData<ModelDataState> modelDataState);
}

As for your second issue with legacy code, that's a fair point, I can see how that could help. Although both of the use cases could be solved by make onCleared non-final, I think we probably should treat them as two separate issues. I'd also be very interested to hear what @milchopenchev and @pettermahlen thinks about these two use cases.

[milchopenchev]

Hey, I had a read through the use cases you described. I'm not entirely opposed to allowing overriding of onCleared, though as mentioned, we've kept it as final on purpose, as it reduces the possibility of leaking the loop.

However, for the first case you describe, where a loop can continue to observe events that occur even in the background, I'm not sure exposing onCleared will help at all. onCleared won't be called when you minimize the app, as the lifecycle owners only go into a paused state - instead it is called only once the lifecycle owner gets destroyed - which is when the entire view model (and thus loop) will get destroyed too. In other words, having access to onCleared won't help you stop observing event sources while your app is minimized.

For an immediate solution to this problem, you can implement an Event in your loop, something like onOwnerPaused/onOwnerResumed - that you have to send from your fragment/activity's onPaused / onResume - which then can modify the model so if you receive further events you know you're in a paused state.

For a solution to the second problem you described, I can definitely see how having that method exposed is better, especially when dealing with legacy code. What we can do perhaps is introduce a completable/single-like emitter in the MobiusLoopViewModel so that anyone can listen on it, and that will emit once and only once a synchronously executed signal when onCleared is called.

I'm leaning more towards using emitters instead of overriding functions, as the MobiusLoopViewModel can work fairly well with Kotlin's typealias without requiring subclassing; and an emitter would still allow a bridge between old code as you described and more modern code.

---

For a long term, built-in solution to the first problem of observing events - I'm not sure what the best way to do this is. @togi's suggestion might work, maybe we don't even necessarily need a live data object. We can pass a class that wraps event sources and automatically filters them (something you can call like so: LifecycleFilter.filter(eventSource: EventSource<T>): EventSource<T>)

The question that comes to mind though is what exactly should we do we want to do when an event occurs in the background? I can think of a few possibilities and I'm not sure which one is best right now:

1. Simply ignore the event (via some sort of filter as @togi described)
2. Queue the events up and then re-send them one at a time once the lifecycle owner is "resumed"
3. Process Events normally and allow model changes, but instead queue Effects, so they're only processed once the lifecycle owner is resumed.

[slesinskik]

@milchopenchev You are right that onCleared itself is not enough to stop/filter observables. However, accessing onCleared could help in the following scenario:

• create and observe observables in ViewModel constructor (without using eventSource() - either due to legacy code, or Function<ViewEffectConsumer<VEF>, MobiusLoop.Factory<MM, EV, EF>> not having access to LiveData state)
• enable/disable filtering of events using onActive/onInactive callbacks
• dispose those observers in onCleared

Without access to onCleared, only observables passed into eventSource() are able to be disposed properly... but those observables don't have access to LiveData state.

Anyway, with regards to long-term solution though, purely due to performance reasons, it would be beneficial for some expensive cold observables (i.e. busy polling, db observers, etc) to be subscribed to only when MobiusLoopViewModel.modelData is active, instead of being subscribed to already when the loop is created and their results (events) being filtered out.

I would like to propose a 4th option (purely because it would be easiest to use in my case):

4. Provide EventSource variant (or a wrapper) that automatically subscribes to user-provided EventSource when MobiusLoopViewModel.modelData becomes active (and unsubscribes when it becomes inactive).

[milchopenchev]

Without access to onCleared, only observables passed into eventSource() are able to be disposed properly... but those observables don't have access to LiveData state.

In order to do that you don't need the LiveData object, but rather the LifecycleOwner object, which you can observe to manually pause the event sources if necessary. (Though as discussed, providing a built-in solution in Mobius would be cleaner)

I think the option 4 you described is what I mentioned for providing a function that wraps event sources, and that it sounds like you expect this wrapper to simply stop propagating all events when the loop is paused, which definitely makes sense for some use cases. I'll have a look at this when I have some free time.

[milchopenchev]

@slesinskik There is now a pull request here: https://github.com/spotify/mobius/pull/136 - to address the above issues. You can read more about the specifics of what was added in the description.

[ffgiraldez]

Hi Mobius team.

We use your lib intensively in my current job, and we ❤️ it; one issue we faced is described here, but I think #137 will fix it, please could you create a new release with those changes?

[pettermahlen]

Apologies for the lack of attention to releasing - but #137 is included in mobius 1.5.5, which is now out on Maven Central. Hope that solves your problem, feel free to reopen if not!