Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MVI] First draft of a reusable architecture #57

Open
wants to merge 50 commits into
base: master
Choose a base branch
from

Conversation

tobiasheine
Copy link
Contributor

Description

This replaces a mvi'ish implementation of a movie search with a reusable implementation of MVI.

Considerations

First things first, this is the first iteration. There is lot's of room for improvements in our implementation of the pattern and for covering more use cases like screen orientation, one time events like (navigation, dialogs, etc.).

Furthermore, this PR introduces documentation around our implementation of MVI and the different components.

In the previous implementation, the SearchResultsViewable and SearchInputViewable did expose callbacks for user actions and methods to render a view state.
The SearchResultsPresenter did register for these callbacks and forwarded the user actions to the SearchResultsModel. Furthermore, it observed the state from that model, mapped it to a view-state and forwarded this to the above-mentioned views.
The SearchResultsModel was responsible to implement the business rules and to expose a stream of state changes according to them.

We replaced the views with a single MVIView<SearchAction, SearchState> which exposes an Observable<Action> and renders a SearchState.
Furthermore, we moved all the business logic into the SearchMiddleware<SearchAction, SearchState, SearchChanges>, which exposes an Observable<SearchChanges>.
These SearchChanges are then observed by the SearchReducer, which maps them to a SearchState.
And finally, we introduced the BaseStore<Action, State, Change> as a use case agnostic class, which collaborates with the above components to connect the different data streams.

Data Flow

High-level Diagram Sequence Diagram
MainView Untitled

Paired with

@gbasile @zegnus @lgvalle @Michal-Novoda

override fun render(state: SearchState) {
when (state) {
is Content -> {
if (searchInput.currentQuery != state.queryString) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you do this check on the view?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question:

TL;DR

Unless we are resuming from an activity destroyed, the textInput would be asked to render the exact same text that is already rendering every-time the user is typing something.
In this case I found that updating the textField too often would result in flashes in the UI and some missing character while typing too fast.

Context

The text in the input field is part of the state, in this way we can example restore it after an Activity has been destroyed and recreated.

That means that for every character introduced in the textInput:

  • An Action is emitted ChangeQuery
  • The middleware emit the relative Change SearchQueryUpdate
  • The Store emits a new State
  • The view is asked to render the new generated state

In complex UI we want to avoid to render changes that are not necessary or that can compromise the User Experience.

IDEALLY we should use some UI-Framework that implement the diffing of the changes to have the best of this architecture, as for example
Litho from Facebook
Domic from Lyft

As a simple workaround we can also simply debounce the ChangeQuery emissions so that we don't flood the components until the user has finished typing

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this doesn't sound like something the view should control, I would vote for debouncing the emissions 👍 this would allow you to change behaviour in the correct place even when the view is changed to a different one. Right now without me asking this question it feels your above explanation is hidden in the view, rather than shown more where control should happen (the intent?/ChangeQuery)

gbasile and others added 13 commits August 23, 2019 10:15
[MVI] Reduce state with state changes
* mvi_reducer:
  Remove not needed null check
  Change changes to be a PublishSubject
  Use scan operator to reduce changes to a state
  Change wording to highlight that the query is updated
  add hideLoading and removeResults actions
  Expect SearchMiddleware to hide progress
  Move domain agnostic screen state and screen state changes from domain specific middleware to the reducer
  Rename domain specific changes to domain agnostic screen state changes

# Conflicts:
#	ModelViewIntentSample/search/src/main/java/com/novoda/movies/mvi/search/domain/SearchDependencyProvider.kt
#	ModelViewIntentSample/search/src/main/java/com/novoda/movies/mvi/search/domain/SearchMiddleware.kt
#	ModelViewIntentSample/search/src/main/java/com/novoda/movies/mvi/search/presentation/SearchActivity.kt
#	ModelViewIntentSample/search/src/test/java/com/novoda/movies/mvi/search/domain/SearchMiddlewareTest.kt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants