Feedback

[sockeqwe]

Hey, your example looks great!

I don’t have much to complain about …
Just some notes that are mostly just a matter of personal prefrences:

• Usually you should prefer switchMap() over flatMap() when dealing with UI Events
• Maybe you can inject the actionProcessor as constructor parameter into your MviViewModel. Or move it into its own .java file. This would reduce the number of lines of your ViewModel quite a bit.
• Maybe observeOn(mSchedulerProvider.ui()) can be moved into View Layer (like Fragment)
• Espresso idling resource is super ugly! I know that all examples in this repository do that, but we can do better!
• You haven’t faced navigation yet, but maybe you will do in the future (`AddEditTaskFragment for example). Navigation is one those questions I have been asked quite often when talking about MVI. I think navigation can be seen us a "side effect" of state transitions. More information here: Navigation in MVi sockeqwe/mosby#261 (comment) . But maybe you have a better or different approach for Navigation.

[oldergod]

Thank you for your feedback!

Usually you should prefer switchMap() over flatMap() when dealing with UI Events

Never thought about the problem this change solves, thanks. This breaks the actual way we handle Espresso idling resource, so need to fix the latter first

Maybe you can inject the actionProcessor as constructor parameter into your MviViewModel. Or move it into its own .java file. This would reduce the number of lines of your ViewModel quite a bit.

I agree. I'd like to do something here.

Maybe observeOn(mSchedulerProvider.ui()) can be moved into View Layer (like Fragment)

I actually think a lot about this.
By keeping it where it is now―before the startWith(Result::IN_FLIGHT), the IN_FLIGHT events are directly emitted into the main thread and can be rendered promptly. Otherwise, we would have to wait for a frame for it to render. Whether are not this one-frame late rendering will be noticed by the user, I guess, depends mostly on the UI state: was the UI moving or not? For static content, e.g. a tap on a button, a response under 100ms should be enough.
By having it moved into the View, all the reducer's work would be handled in the background. Not really a problem in this sample but I had some when dealing with long listed recyclerview when some work had to be done on the data list. Not sure about the efficiency but handling thread such as repo call ⇒ io(), reducer ⇒ computation() and rendering ⇒ MainThread() would make sense.
I don't know how to make this decision though.

Did you have some other consideration in mind?

Espresso idling resource is super ugly! I know that all examples in this repository do that, but we can do better!

Yes, did not give it much thought so far.

You haven’t faced navigation yet, but maybe you will do in the future (`AddEditTaskFragment for example). Navigation is one those questions I have been asked quite often when talking about MVI. I think navigation can be seen us a "side effect" of state transitions.

Grey zone here... I'll tackle this problem with AddEditTask.

[malmstein]

Agree on the comments above, and I really like the idea of an class that deals with Navigation. Since we have here tablet and phone layouts to consider here, Navigator could be an interface that is implemented by a PhoneNavigator and TabletNavigator. Does that make sense?

[oldergod]

Does that make sense?

AddEditTask's navigation would mean save task ->return with result. What would change between the two implementation you mentioned? As well, if I'm not mistaken, the current TODO samples do not change their UI/UX based on the device's size, is it something you'd like to address?

[malmstein]

There's a branch that covers tablet but you are right, this sample doesn't need to!

Actually, AddEditTask's navigation would start in the previous Activity, which should also have a Navigator. The return with result part would be the logic inside the AddEditTask's Navigator .