Use RX semantics for fragment interactions? about ground-android HOT 12 CLOSED

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

from ground-android.

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.

from ground-android.

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.

from ground-android.

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.

from ground-android.

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.

from ground-android.

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.

from ground-android.

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

from ground-android.

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.

from ground-android.

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.

from ground-android.

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.

from ground-android.

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).

from ground-android.

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. 🙄

from ground-android.