Added ToggledEventSource and added it to the MobiusLoopViewModel #137

milchopenchev · 2020-10-09T06:39:04Z

Added a protected onClearedInternal method in the MobiusLoopViewModel that can be overriden and will be called by the onCleared method just before the loop is shut down
Added an (internal) ObservableMutableLiveData class that allows its state (active/inactive) to be observed as a `EventSource
Added new MobiusLoopViewModel creator function that supplies a EventSource that's tied to the ViewModel's getModels(): LiveData<M> state (specifically whether it has any active observers or not)
Added a utility class ToggledEventSource which forwards values from a target event source, and can additionally be controlled whether to forward those values from a secondary Boolean-emitting toggling-event-source.

codecov · 2020-10-09T07:07:54Z

Codecov Report

❗ No coverage uploaded for pull request base (master@a588f8a). Click here to learn what that means.
The diff coverage is 95.74%.

@@            Coverage Diff            @@
##             master     #137   +/-   ##
=========================================
  Coverage          ?   79.35%           
  Complexity        ?      337           
=========================================
  Files             ?       87           
  Lines             ?     1773           
  Branches          ?      113           
=========================================
  Hits              ?     1407           
  Misses            ?      337           
  Partials          ?       29

Impacted Files	Coverage Δ	Complexity Δ
.../com/spotify/mobius/extras/ToggledEventSource.java	`95.74% <95.74%> (ø)`	`13.00 <13.00> (?)`
...main/java/com/spotify/mobius/rx2/Transformers.java	`100.00% <0.00%> (ø)`	`6.00% <0.00%> (?%)`
...main/java/com/spotify/mobius/rx3/RxMobiusLoop.java	`100.00% <0.00%> (ø)`	`2.00% <0.00%> (?%)`
...om/spotify/mobius/internal_util/ImmutableUtil.java	`100.00% <0.00%> (ø)`	`5.00% <0.00%> (?%)`
...xtras/connections/MergeConnectablesConnection.java	`92.85% <0.00%> (ø)`	`8.00% <0.00%> (?%)`
...in/java/com/spotify/mobius/rx3/RxEventSources.java	`45.45% <0.00%> (ø)`	`1.00% <0.00%> (?%)`
.../src/main/java/com/spotify/mobius/MobiusStore.java	`100.00% <0.00%> (ø)`	`3.00% <0.00%> (?%)`
...ain/java/com/spotify/mobius/rx/RxEventSources.java	`48.14% <0.00%> (ø)`	`1.00% <0.00%> (?%)`
...ava/com/spotify/mobius/ControllerStateRunning.java	`92.85% <0.00%> (ø)`	`8.00% <0.00%> (?%)`
...java/com/spotify/mobius/rx2/MergedTransformer.java	`100.00% <0.00%> (ø)`	`2.00% <0.00%> (?%)`
... and 78 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a588f8a...24a4915. Read the comment docs.

togi · 2020-10-16T13:57:26Z

mobius-android/src/main/java/com/spotify/mobius/android/ObservableMutableLiveData.java

+/** An extension of MutableLiveData that allows its Active/Inactive state to be observed. */
+final class ObservableMutableLiveData<T> extends MutableLiveData<T>
+    implements EventSource<Boolean> {
+  private final List<Consumer<Boolean>> stateListeners = new ArrayList<>();


i think this would do better to be a CopyOnWriteArrayList instead, and remove the synchronization blocks

Hmm, I can convert it to that, my only concern (and why there's a copy done in notifyListeners is someone deciding to unsubscribe in the middle of a notify back call - so it would modify the array while its being iterated over. I'm not sure if a CopyOnWriteArrayList would fix this, would it? Edit: I experimented around, i think a CopyOnWriteArrayList works fine.

So I did experiment around with this a little, seemed to work fine but I think maybe I still need to copy the array before sending since something might remove itself from the array while iterating, and that might cause issues.

From the COWAL docs: the iterator is guaranteed not to throw ConcurrentModificationException. The iterator will not reflect additions, removals, or changes to the list since the iterator was created. That class is super useful for cases like listeners/observers because it has the semantics you usually want, and it's generally better than any implementation you can come up with yourself. :)

mobius-android/src/main/java/com/spotify/mobius/android/MobiusLoopViewModel.java

mobius-android/src/test/java/com/spotify/mobius/android/ObservableMutableLiveDataTest.java

togi · 2020-10-16T14:04:44Z

mobius-core/src/main/java/com/spotify/mobius/ConditionalFilteringEventSource.java

+  @Nonnull private final Function<T, Boolean> condition;
+
+  @Nonnull
+  final List<WeakReference<FilteredEventSource<?>>> filteredEventSources = new ArrayList<>(1);


This should probably also be a CopyOnWriteArrayList?

Yeah, this one should be fine to convert.

togi · 2020-10-16T14:14:39Z

mobius-core/src/main/java/com/spotify/mobius/FilteringEventSource.java

+ *
+ * @param <T> The event class
+ */
+public interface FilteringEventSource<T> extends EventSource<T> {


I'm not a huge fan of calling this "filtering". I associate that word much stronger with just ignoring some events, and not starting/stopping something based on a condition. In Rx (JS/Java/C#/etc) this kind of behavior is referred to as "switching" (technically it's a special case of switchMap). I don't know if it's the perfect name for us, but I believe "filter" is usually one of the most well defined operators when it comes to reactive functional programming, and the classes here that use the name "filter" aren't following that definition.

Hm, true, this doesn't filter on a per-event basis. Happy to rename it to something like SwitchMappingEventSource ? or better name if you have any suggestions?

I don't think this interface is needed. What seems to be happening is:

EventSource A produces data that shouldn't always be passed along.

EventSource B produces some other data that determines whether A's data should be thrown away.

So what's needed is like an operator like

(EventSource<A>, EventSource<B>, Function<B, Boolean>) -> EventSource<A>

I think it's like a 'gate' in electronics, right? Only open if the signal on 'B' evaluates to true. Maybe that's what it should be called? Something like:

EventSource<A> gated = EventSources.gatedBy(eventSourceB, conditionOnB).apply(eventSourceA);

togi · 2020-10-16T14:15:51Z

mobius-core/src/main/java/com/spotify/mobius/FilteredEventSource.java

+ *
+ * @param <F> The Event class
+ */
+final class FilteredEventSource<F> implements EventSource<F> {


How come this is a separate thing from ConditionalFilteringEventSource? Did it end up being very complex?

Yes, it seemed to also mix responsibilities a little. Originally it was an internal non-static class in ConditionalFilteringEventSource, but when writing tests i realized how big it was getting, and that really I was testing two separate things at once - the wrapping around some decider-event-source + forwarding its events, and the actual managing of subscribe/unsubscribe from to-be-filtered event sources. Splitting it seemed to make sense, and tests became smaller and more readable.

pettermahlen

I think we should talk a bit about names, maybe this is more easily done offline.

pettermahlen · 2020-10-22T09:39:26Z

mobius-core/src/main/java/com/spotify/mobius/FilteringEventSource.java

+ *
+ * @param <T> The event class
+ */
+public interface FilteringEventSource<T> extends EventSource<T> {


I don't think this interface is needed. What seems to be happening is:

EventSource A produces data that shouldn't always be passed along.

EventSource B produces some other data that determines whether A's data should be thrown away.

So what's needed is like an operator like

(EventSource<A>, EventSource<B>, Function<B, Boolean>) -> EventSource<A>

I think it's like a 'gate' in electronics, right? Only open if the signal on 'B' evaluates to true. Maybe that's what it should be called? Something like:

EventSource<A> gated = EventSources.gatedBy(eventSourceB, conditionOnB).apply(eventSourceA);

pettermahlen

Some comments and questions, good changes overall!

pettermahlen · 2020-11-02T12:56:55Z

mobius-android/src/main/java/com/spotify/mobius/android/MobiusLoopViewModel.java

@@ -75,9 +75,24 @@ protected MobiusLoopViewModel(
      @Nonnull Init<M, F> init,
      @Nonnull WorkRunner mainLoopWorkRunner,
      int maxEffectQueueSize) {
+    this(
+        (Consumer<V> viewEffectConsumer, EventSource<Boolean> activeModelFilter) ->


nit:

Suggested change

(Consumer<V> viewEffectConsumer, EventSource<Boolean> activeModelFilter) ->

(Consumer<V> viewEffectConsumer, EventSource<Boolean> activeModelEventSource) ->

mobius-android/src/test/java/com/spotify/mobius/android/ObservableMutableLiveDataTest.java

pettermahlen · 2020-11-02T13:18:05Z

mobius-extras/src/main/java/com/spotify/mobius/extras/ToggledEventSource.java

+      boolean initialToggleState) {
+    final ToggledEventSource<E> toggledEventSource =
+        new ToggledEventSource<>(targetEventSource, initialToggleState);
+    new TogglingSourceObserver(togglingSource, toggledEventSource);


I don't understand this implementation - is the idea that when the wrapped event source gets garbage collected, then the TogglingSourceObserver should stop subscribing to it? (as in, dispose its subscription). Can that even happen? Is it possible to write this in a more explicit way - if not, could there be some more documentation on how it works and why that solution was chosen?

Kind of, yes. The idea was to not keep the toggledEventSource (which is returned to the user) alive unless there's something else that has a reference to it. That's how I expect stand-alone EventSource objects to exist - only while something else can reach them.

My reasoning is like this: since this object (the TogglingSourceObserver) subscribes to the togglingSource at all times, that means the togglingSource would keep this object in memory, which would then keep the toggledEventSource alive too (unless I make it WeakRef'd). That would not make sense since there's no way to reach the toggledEventSource in any way from the togglingEventSource.
With the weak ref, at least I can do some memory cleanup the next time the togglingEventSource changes true/false state.

I agree I should try to add some documentation to the constructor to make it clearer, and I'll check to make sure it isn't causing any issues.

I also updated the name of the class, plus added some extra documentation to it, even though its internal.

pettermahlen · 2020-11-02T13:26:12Z

mobius-extras/src/test/java/com/spotify/mobius/extras/ToggledEventSourceTest.java

+    dataEventSource.emit("a");
+
+    dataEventSource.assertConsumerCount(1);
+    assertThat(received.size(), equalTo(1));


A little tip: you can also use assertj here:

org.assertj.core.api.Assertions.assertThat(received).containsExactly("a");

pettermahlen · 2020-11-02T13:27:48Z

mobius-extras/src/test/java/com/spotify/mobius/extras/ToggledEventSourceTest.java

+    dataEventSource.emit("b");
+
+    dataEventSource.assertConsumerCount(0);
+    assertThat(received.size(), equalTo(0));


and here (with an import of the right assertThat), you could write:

assertThat(received).isEmpty();

it's a matter of taste, but I quite like AssertJ - and Truth even better, but we're currently using AssertJ in Mobius.

pettermahlen · 2020-11-02T13:29:11Z

mobius-extras/src/test/java/com/spotify/mobius/extras/ToggledEventSourceTest.java

+  }
+
+  @Test
+  public void testThatToggledEventSourceIsNotKeptAliveByUUnderlyingDataSource() {


Does this test still work if you create a subscription?

~~Yes, it does - just tested. If you subscribe to an EventSource, but don't have a direct reference, it should be free to be GC'd - right?~~ Actually need to retest, seems to somewhat work if the TogglingEventSource is toggled after the =null

Ok, figured it out, was being kept alive by the Disposable, so once its disposed, it should not be kept alive by a subscription.

…a Boolean-emitting EventSource

milchopenchev · 2021-01-20T11:04:40Z

Hi @togi @pettermahlen - So this PR has been around since last year, I wanted to figure out how to move it forward, either close it, or merge it. Any thoughts?

togi · 2021-01-21T08:30:58Z

Tbh I thought this was merged already, sorry about that. Looking through it, I don't see any problems with the PR and it seems to me like all the comments have been addressed, so I don't see any reason not to merge it.

milchopenchev force-pushed the milchop-mobius-loop-viewmodel-filtering-and-on-cleared-v2 branch from 81effc4 to ba02e34 Compare October 9, 2020 07:04

milchopenchev force-pushed the milchop-mobius-loop-viewmodel-filtering-and-on-cleared-v2 branch from ba02e34 to 71b6159 Compare October 9, 2020 07:12

togi reviewed Oct 16, 2020

View reviewed changes

mobius-android/src/main/java/com/spotify/mobius/android/MobiusLoopViewModel.java Show resolved Hide resolved

togi reviewed Oct 16, 2020

View reviewed changes

mobius-android/src/test/java/com/spotify/mobius/android/ObservableMutableLiveDataTest.java Show resolved Hide resolved

togi reviewed Oct 16, 2020

View reviewed changes

milchopenchev force-pushed the milchop-mobius-loop-viewmodel-filtering-and-on-cleared-v2 branch 2 times, most recently from aea1b6b to 853cd03 Compare October 17, 2020 13:10

pettermahlen requested changes Oct 22, 2020

View reviewed changes

milchopenchev force-pushed the milchop-mobius-loop-viewmodel-filtering-and-on-cleared-v2 branch 2 times, most recently from 2b313f3 to 28aae27 Compare October 23, 2020 13:26

milchopenchev changed the title ~~Added FilteringEventSource and added it to the MobiusLoopViewModel~~ Added ToggledEventSource and added it to the MobiusLoopViewModel Oct 23, 2020

togi previously approved these changes Oct 23, 2020

View reviewed changes

milchopenchev requested a review from pettermahlen October 26, 2020 12:07

pettermahlen reviewed Nov 2, 2020

View reviewed changes

milchopenchev dismissed togi’s stale review via fbb6b6c November 4, 2020 15:03

milchopenchev force-pushed the milchop-mobius-loop-viewmodel-filtering-and-on-cleared-v2 branch from 28aae27 to fbb6b6c Compare November 4, 2020 15:03

Added a Toggled Event Source and changed MobiusLoopViewModel to send …

24a4915

…a Boolean-emitting EventSource

milchopenchev force-pushed the milchop-mobius-loop-viewmodel-filtering-and-on-cleared-v2 branch from fbb6b6c to 24a4915 Compare November 5, 2020 10:27

milchopenchev requested a review from pettermahlen November 5, 2020 13:10

togi approved these changes Jan 21, 2021

View reviewed changes

togi merged commit ae3ee24 into spotify:master Jan 26, 2021

ffgiraldez mentioned this pull request Sep 14, 2021

Replacefinal modifier with @CallSuper annotation for MobiusLoopViewModel.onCleared #135

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added ToggledEventSource and added it to the MobiusLoopViewModel #137

Added ToggledEventSource and added it to the MobiusLoopViewModel #137

milchopenchev commented Oct 9, 2020 •

edited

Loading

codecov bot commented Oct 9, 2020 •

edited

Loading

togi Oct 16, 2020

milchopenchev Oct 16, 2020 •

edited

Loading

milchopenchev Oct 17, 2020

pettermahlen Oct 18, 2020

togi Oct 16, 2020

milchopenchev Oct 16, 2020

togi Oct 16, 2020

milchopenchev Oct 16, 2020

pettermahlen Oct 22, 2020

togi Oct 16, 2020

milchopenchev Oct 16, 2020

pettermahlen left a comment

pettermahlen Oct 22, 2020

pettermahlen left a comment

pettermahlen Nov 2, 2020

pettermahlen Nov 2, 2020

milchopenchev Nov 4, 2020

milchopenchev Nov 5, 2020

pettermahlen Nov 2, 2020

pettermahlen Nov 2, 2020

pettermahlen Nov 2, 2020

milchopenchev Nov 4, 2020 •

edited

Loading

milchopenchev Nov 4, 2020

milchopenchev commented Jan 20, 2021

togi commented Jan 21, 2021

	(Consumer<V> viewEffectConsumer, EventSource<Boolean> activeModelFilter) ->
	(Consumer<V> viewEffectConsumer, EventSource<Boolean> activeModelEventSource) ->

Added ToggledEventSource and added it to the MobiusLoopViewModel #137

Added ToggledEventSource and added it to the MobiusLoopViewModel #137

Conversation

milchopenchev commented Oct 9, 2020 • edited Loading

codecov bot commented Oct 9, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

milchopenchev Oct 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pettermahlen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pettermahlen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milchopenchev Nov 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milchopenchev commented Jan 20, 2021

togi commented Jan 21, 2021

milchopenchev commented Oct 9, 2020 •

edited

Loading

codecov bot commented Oct 9, 2020 •

edited

Loading

milchopenchev Oct 16, 2020 •

edited

Loading

milchopenchev Nov 4, 2020 •

edited

Loading