Use RX semantics for fragment interactions?

[scolsen]

Yet another stylistic/preferential question--forgive me for having so many of these but once they're firmly answered I'll move forward without causing any trouble!

We've recently had some discussions about naming subjects that track user interactions appropriately such as (addFeatureClicks) etc. Most of these currently (or will) live in viewmodels as subjects. However, I think it may be more appropriate to track interactions (clicks, etc) in the fragment, granting the view models some level of independence.

This is already the case in some sense, but we currently often mix RX methods and approaches with more traditional direct VM method calls etc. Instead, we can stick to RX as much as possible, creating observables/subjects to track fragment interactions and capturing responses to view model data as transformations on these interactions.

see 4491816 experimental/scolsen/observable-views for an example using this approach.

For reference, here's a relevant snippet from the previously mentioned commit:

First, we specify the appropriate fragment reaction in the onCreate method and subscribe to the stream:

  @Override
  public void onCreate(@androidx.annotation.Nullable Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);
    saveClicks = PublishSubject.create();
    singleSelectDialogFactory = new SingleSelectDialogFactory(getContext());
    multiSelectDialogFactory = new MultiSelectDialogFactory(getContext());
    viewModel = getViewModel(EditRecordViewModel.class);

    Observable<Boolean> saveClickReactions =
        saveClicks
            .map(__ -> viewModel.onSaveClick())
            .doOnNext(
                result -> {
                  if (!result) {
                    EphemeralPopups.showFyi(getContext(), R.string.no_changes_to_save);
                    navigator.navigateUp();
                  }
                });

    saveClickReactions.subscribe();

Second, when a user clicks, drags, w/e, we emit:

  @OnClick(R.id.save_record_btn)
  void onSaveClick() {
    saveClicks.onNext(new Object());
  }

Note that this is mostly a stylistic difference and it may be totally unnecessary if we're able to switch every fragment/VM data coupling to LiveData bindings. This approach does grant us some additional flexibility in that it's now fairly straightforward to accumulate further transformations/interactions on the initial stream, and it eliminates the need to track any state for dependent reactions (if any exist, we can simply model them as transformations, merges, etc.).

It also frees up

[gino-m]

forgive me for having so many of these

Nothing to forgive, these are very productive foundational questions that we'd do well to address sooner than later!

I think it may be more appropriate to track interactions (clicks, etc) in the fragment, granting the view models some level of independence.

Iiuc, in MVVM the idea of the View Model is to abstract the implementation details of the view, but not the details of interactions; conversely, the View Model is intended to be a model of the interactions between the user and the UI, modulo Android-specific "View" details. So in my understand, hiding some of the view behaviors might be considered a bug and not a feature.

Also, in many cases, user interactions require some UI change after some asynchronous event completes. In your example, we'd be navigating away from the Fragment and saying "Saved" before the save action actually completes. Since the async operations are kicked off by VM, it seems somewhat arbitrary to have sync. side effects handled in the View, and async. ones in the VM. It also spreads out reactions to user interactions among the View and View Model, making the logic harder to follow, and also making interactions handled inside the view almost impossible to test.

Does that make sense?

Btw, the BasicRx example also seems to have the problem of accumulating subscriptions; just filed android/architecture-components-samples#615 to verify if this is in fact an issue.

[gino-m]

Correction: Looking at the delta in your change, I see now that the message is shown when there's nothing to save, and that it's already done that way in the current implementation. The points in my previous reply still apply, and that we should eventually refactor that logic to pass through the VM as part of Issue #9.

[scolsen]

Ahha! Thanks for the clarification. It would certainly be silly to go against the principles of our architecture!

This makes sense. What's nice is that we can use streams to capture interactions in either case. Based on your explanation, keeping the interactions in the VM makes sense to me now.

I suppose what I'm really hoping to do is simplify the connection between the View and View Model in some standardized fashion. It'd be nice if we can eliminate some of the <T>Subjects that act as intermediaries of sorts between the V and VM. I suppose the best and most appropriate way of doing that is to use LiveData binding when possible.

[gino-m]

I wish there were a more succinct and readable way to model these. The reason we end up with lots of Subjects is because we're trying to avoid any references from the ViewModel to the View. Passing the ViewModel a callback or stream of clicks would violate this. Also, using LiveData instead of Rx wouldn't reduce the number of streams needed, since we'd just end up replacing each Subject with an analagous MutableLiveData. So basically I think the number of Subjects is an artifact of the clean MVVM architecture, not Rx per se.

That said, Rx stream chains can get pretty hairy, so we should try to mitigate that, for example by always putting actual business logic in methods rather than on lambdas in the chains.

[gino-m]

Would it be helpful to just declare Subjects as public final, and to allow Views to invoke onNext() directly? We're not encapsulating away the fact that Rx is being used, so it wouldn't break any existing abstractions, and it would obviate the need to boilerplate methods that just call onNext. Then exposed Subjects would become quasi method objects, à la:

viewModel.addFeatureClicks.onNext(NIL);

Or even better, databinding could call onNext directly.

Another observation: we should take care not to pass Views or Fragment references into the ViewModel streams for the reasons mentioned above, so perhaps streams without an object should be strongly typed (we can create a Nil class or just use Boolean for example) to prevent accidental misuse.

[gino-m]

Re Subjects with no value, see https://stackoverflow.com/questions/47992689/rxjava2-subject-with-no-data

[scolsen]

Would it be helpful to just declare Subjects and public final, and to allow Views to invoke onNext directly? We're not encapsulating away the fact that Rx is being used, so it wouldn't break any existing abstractions, and it would obviate the need to boilerplate methods that just call onNext. Then exposed Subjects would become quasi method objects, à la:

viewModel.addFeatureClicks.onNext(NIL);

Or even better, databinding could call onNext directly.

Another observation: we should take care not to pass Views or Fragment references into the ViewModel streams for the reasons mentioned above, so perhaps streams without an object should be strongly typed (we can create a Nil class or just use Boolean for example) to prevent accidental misuse.

I do like that approach! Another option, I think, for the cases in which we pass no/dummy values is to use a connectable observable. I think the semantics of this are pretty close to what we're actually trying to do, namely, kick-off emissions at a certain point in time. I think a subject makes more sense when we cannot initialize a value in the VM (e.g. we emit some runtime value from a field or something from the View). If I'm understanding connectables correctly, the benefit to this is that we can handle "dummy values" entirely in the VM. The view doesn't need to know anything about some dummy value contract, it would just call viewModel.observable.connect() at the appropriate time. It should work appropriately as long as we're doing our subscriptions in the constructor and calling connect after VM construction.

Creating some kind of utility Signal enum is also a good solution! I'm open to either strategy.

[scolsen]

Hm, scratch that re: connectables, that actually wouldn't work.

I believe the simplest and best case is what you've suggested, creating some dummy signal and calling onNext directly in the view.

[gino-m]

Yeah, ConnectableObservable appears to serve a different purpose.

My initial thought would be to create a Nil enum with one void value, NIL, but that then requires the layout xml to import the symbol to reference it. Creating a helper class that extends PublishSubject<Nil> and provides a no-arg onNext() might be a sol'n. I general relying on inheritance for util functions can lead to headaches, but this use is tightly scoped enough that it's probably fine. The question is whether having a placeholder object + helper object + calls to onNext() in layout is really better than just adding named onFooClick() method to the ViewModel. All things considered I'm leaning towards the latter option.

[scolsen]

Keeping it as is and having the onFooClick() methods would ultimately be more flexible, I think. If we had to update what happens on a click with some additional logic, we would simply add a couple of lines to the method, whereas if we made direct calls to onNext in the view and used some enum we lose some of that flexibility--we'd have to implement additional logic in the subject stream itself which gets complicated if we want to fire off multiple streams based on one click or if we need to do some complex preparatory work on the argument to onNext (in those cases in which it's not nil)--basically, we'd wind up needing a wrapper method anyway in the view to call each onNext (one might suspect we probably shouldn't have situations in which we need to kick off two separate subjects from a single interaction anyway, but you never know).

We can still use some dummy enum in VMs to make things extra clear, or just Object or Result<Object> (if we have some situation in which the call can somehow yield an error).

So, I guess what we're looking at is something like:

enum Interaction {
  DONE 
}

PublishSubject<Interaction> fooBarClicks/drags/etc)s = PublishSubject.create();

// do some stream manipulation.

public void onFooBar<Interaction>() {
  fooBarClicks(/drags/etc)s.onNext(Interaction.DONE);
} 

// A concrete example

PublishSubject<Interaction> addFeatureClicks = PublishSubject.create();

// so some stream manipulation

public void onAddFeatureClick() {
  addFeatureClicks.onNext(Interaction.DONE)
}

I don't know what the best name for the interaction enum and member would be, some ideas: Nil.NIL, Interaction.INTERACTION, Interaction.DONE, Nothing.NOTHING, Signal.SIGNAL, Interaction.SIGNAL, UI.Interaction

[scolsen]

Another option is to go all in on defining an interaction types and wrappers. For example, we could create a Click which in certain cases carries additional data (such as coordinates). (though I imagine Android might already provide something like this).

[gino-m]

Keeping it as is and having the onFooClick() methods would ultimately be more flexible, I think.

+1. Looks like we've come full circle on this, but I feel more confident it's the right path now that we've fleshed out the alternatives.

I don't know what the best name for the interaction enum and member would be,

I'm slightly partial to Nil.NIL, since it's clear, concise, doesn't conflict with other Java reserved symbols, and it's at the right level of abstraction (Rx streams vs. View logic). It probably belongs in the ..gnd.rx package.

Another option is to go all in on defining an interaction types and wrappers.

I think this would lead to a small combinatoric explosion of Subject types x event types, which is already well served by generics.

For example, we could create a Click which in certain cases carries additional data (such as coordinates). (though I imagine Android might already provide something like this).

We have Point value object to represent these, so Subject<Point> is correct, clear, and to the point. 🙄