Open artem-zinnatullin opened 8 years ago
thanks for the great podcast! could you share the repository which Hannes said is a good mvvm example?
Here you go: https://github.com/rheinfabrik/android-mvvm-example
As already said, it's build with RxJava instead of android data-binding engine, but it should be possible to understand the "big picture" without having a good rx java knowledge.
Btw. I have sent a pull request with the missing links to the show note
Jeez, I forgot to add a bunch of links including links to Hannes profiles and projects (it was 7 am..), will do as soon as possible!
@yshrsmz thank you for the feedback :)
Good job, don`t stop.
@artem-zinnatullin how do you keep background tasks execution during orientation changes without saving presenter?
@AdamCopperfield usually apps I write has almost full offline support so they have kind of persistant queue for operations that need to be done, even if they were started offline, + local db or files for storing content. So in case of orientation change I just need to save id
of the operation or content I need to display.
But sometimes when you need to do request without using such queue I'd use replay()
operator from RxJava and persist the Observable
with or without Dagger via retained Fragment
or Activity.getLastNonConfigurationInstance()
, if you don't use Rx, you can apply similar technique for any kind of callback listeners.
@atetc thanks :)
Great talk about modern topic, don't stop.
Great podcast! I am working on an MVVM implementation and I think this pattern is great! You are right, using it you move all the logic from the Activity to the ViewModel but I don't agree that the ViewModel contains spaghetti code. It's pure Java code, so it's easy to write JUnit test and refactor it. And you can create custom attributes (thanks to BindingAdapter annotation) to avoid complex UI code in xml or in ViewModel. The framework I am working on is this: https://github.com/fabioCollini/mv2m, would be great to hear your feedback! :)
So after seeing that @artem-zinnatullin is using locals db or some persistence anyway in his apps, I can understand why things might work for him.
onSaveInstanceState
to not have to do everything all over again.
Well technically you could think of all kind of ways to do that, but this a quite convinient way for multiple reasons.First of all, thanks again for your mention and the kind words :) Let me quickly go through a few points both of you made:
Now on to the most important aspect you mentioned:
MVVM does not scale well.
I can prove you wrong here :D When used correctly ViewModels can be used for big to huge applications. An example can be seen in BVB and trivago. Both application heavily use the Model View ViewModel pattern. The main thing to keep in mind is, that ViewModels are not the Silver Bullet to everything. They won't be fixing rotation issues, your app's communication flow etc. They simply are an additional layer between your view and some data. With that in mind you can build small, composable ViewModels for your application.
@fabioCollini That's a nice library. I just have some nitpicking feedback: What I've meant with spaghetti code is the lack of separation of concerns. I think you can see a good example here:
public class NoteViewModel extends ViewModel<String, NoteModel> {
private final Executor backgroundExecutor;
private final Executor uiExecutor;
private NoteLoader noteLoader;
private NoteSaver noteSaver;
private MessageManager messageManager;
public final ObservableBoolean loading = new ObservableBoolean();
public final ObservableBoolean sending = new ObservableBoolean();
public NoteViewModel(Executor backgroundExecutor, Executor uiExecutor, NoteLoader noteLoader, NoteSaver noteSaver, MessageManager messageManager) {
this.backgroundExecutor = backgroundExecutor;
this.uiExecutor = uiExecutor;
this.noteLoader = noteLoader;
this.noteSaver = noteSaver;
this.messageManager = messageManager;
}
@NonNull @Override public NoteModel createModel() {
return new NoteModel();
}
@Override public void resume() {
if (!loading.get() && !model.isLoaded() && getArgument() != null) {
reloadData();
}
}
public void reloadData() {
loading.set(true);
backgroundExecutor.execute(new Runnable() {
@Override public void run() {
executeServerCall();
}
});
}
private void executeServerCall() {
try {
final Note note = noteLoader.load(getArgument());
uiExecutor.execute(new Runnable() {
@Override public void run() {
model.update(note);
loading.set(false);
}
});
} catch (Exception e) {
uiExecutor.execute(new Runnable() {
@Override public void run() {
model.getError().set(true);
loading.set(false);
}
});
}
}
public void save() {
boolean titleValid = checkMandatory(model.getTitle(), model.getTitleError());
boolean textValid = checkMandatory(model.getText(), model.getTextError());
if (titleValid && textValid) {
sending.set(true);
backgroundExecutor.execute(new Runnable() {
@Override public void run() {
try {
Note note = new Note(null, model.getTitle().get(), model.getText().get());
String noteId = model.getNoteId();
if (noteId == null) {
noteId = noteSaver.createNewNote(note).getObjectId();
model.setNoteId(noteId);
} else {
noteSaver.save(noteId, note);
}
hideSendProgressAndShoMessage(R.string.note_saved);
} catch (RetrofitError e) {
hideSendProgressAndShoMessage(R.string.error_saving_note);
}
}
});
}
}
private void hideSendProgressAndShoMessage(final int message) {
uiExecutor.execute(new Runnable() {
@Override public void run() {
messageManager.showMessage(activityHolder, message);
sending.set(false);
}
});
}
private boolean checkMandatory(ObservableString bindableString, ObservableInt error) {
boolean empty = bindableString.isEmpty();
error.set(empty ? R.string.mandatory_field : 0);
return !empty;
}
@Override public ActivityResult onBackPressed() {
return new ActivityResult(true, new Note(model.getNoteId(), model.getTitle().get(), model.getText().get()));
}
}
This NoteViewModel
is huge and has a lot of responsibilities: Licecycle, Navigation (onBackPressed()), than it interacts with more than one "business logic" like MessageManager
, NoteLoader
, NoteServer
. You also change the state That's what I mean when I'm talking about shifting spaghetti code from Activity to ViewModel. You might could split this responsibilities into more decoupled components (like commands, controller, etc.). Keep up your good work!
@Gi-lo Regarding scaling, you are right MVVM can scale and you have proved that. I should have said that I, personally, am not able to scale it when working with XML bindings. It didn't worked good for me on Windows Phone and also with android data binding engine it's a little bit ugly. IMHO, it works for quickly building bring some two way data binding small UI things together like EditText
. That is my definition of a MVVM (just for a small UI element). But having one ViewModel managing i.e. RecyclerView and ProgressBar is from my point of view not ideal. MVVMC makes that slightly better and allows sharing "model" between ViewModels. But overall, YES, I have to agree MVVM can scale
@panzerdev Yes, usually I prefer having multiple activities. As you have already said, Intents are fundamental in Android especially for deep linking. Actually, that is one things that I wish fragments would support somehow out of the box. However, we can build also a decoupled component that is responsible for navigation and deep link resolving. The nice thing of one activity is that you can animate your views much smoother as the user navigates through your app from "screen" to "screen", i.e. TransitionManager
works like a charm with almost zero code. Sure we could do that with ActivityTransitions
but you will notice (especially on low end device) a delay when starting the new Activity. Nevertheless, usually i also prefer having multiple Activities over one single Activity with lot of fragments.
@panzerdev
savedInstanceState
Thank you for feedback!
// Interesting discussions, btw, same story as @sockeqwe, not sure I'll be able to scale MVVM project on Android.
@sockeqwe you are right, that class is a mess :( But one reason is that the demo project doesn't use RxJava to keep it simple. Using RxJava the same class can be written in a better way:
public class NoteViewModel extends RxViewModel<String, NoteModel> {
private NoteLoader noteLoader;
private NoteSaver noteSaver;
private MessageManager messageManager;
public final ObservableBoolean loading = new ObservableBoolean();
public final ObservableBoolean sending = new ObservableBoolean();
public NoteViewModel(SchedulerManager schedulerManager, NoteLoader noteLoader, NoteSaver noteSaver, MessageManager messageManager) {
super(schedulerManager);
this.noteLoader = noteLoader;
this.noteSaver = noteSaver;
this.messageManager = messageManager;
}
@NonNull @Override public NoteModel createModel() {
return new NoteModel();
}
@Override public void resume() {
if (!loading.get() && !model.isLoaded() && getArgument() != null) {
reloadData();
}
}
public void reloadData() {
subscribe(
loading::set,
noteLoader.load(getArgument()),
model::update,
t -> model.getError().set(true)
);
}
public void save() {
boolean titleValid = ValidationUtils.checkMandatory(model.getTitle(), model.getTitleError());
boolean textValid = ValidationUtils.checkMandatory(model.getText(), model.getTextError());
if (titleValid && textValid) {
subscribe(
sending::set,
noteSaver.save(model.getNoteId(), model.getTitle().get(), model.getText().get()),
note -> messageManager.showMessage(activityHolder, R.string.note_saved),
t -> messageManager.showMessage(activityHolder, R.string.error_saving_note)
);
}
}
@Override public ActivityResult onBackPressed() {
return new ActivityResult(true, new Note(model.getNoteId(), model.getTitle().get(), model.getText().get()));
}
}
Maybe it continues to have too many responsibilities but it's not difficult to refactor it to delegate to other objects (for example something like a Use Case in clean architecture). How do you solve this problem in MVP? I think that MVP with passive view and this version of MVVM (I am not sure this is a real MVVM implementation) are very similar; the main difference is that in a presenter you call a method on the view to update it while in the View Model you update the model and then the data binding framework will accordingly update the view.
Please, turn off skipping silence in next records. Subsecond gaps makes it difficult to understand.
Yeah, I understand, actually episode was about 1h 20m and we discussed some other things but decided to make it shorter and my audio editing skills are not good enough yet :)
But, at the same time episode was very informative, I hope, haha :)
On Tue, 9 Feb 2016, 23:16 Dmitry notifications@github.com wrote:
Please, turn off skipping silence in next records. Subsecond gaps makes it difficult to understand.
โ Reply to this email directly or view it on GitHub https://github.com/artem-zinnatullin/TheContext-Podcast/issues/1#issuecomment-182044263 .
@artem_zin
@artem-zinnatullin For the first Podcast it was quite good and understandable. Another thing: I think you should open an issue for topic requests.
@Gi-lo definitely will create an issue for suggestions for the next episode soon!
thanks for the updates!(sorry to be late
I have some questions. You said sometimes you have several presenters in one activity:
You said sometimes you have several presenters in one activity
Yes, take gmail for example:
What kind of communication do you have? My presenters doesn't communicate with each other (and I don't know what they should say to each other :smile: ) . I guess you mean something like when a mail from MailView
has been marked as favorite the MailPresenter
have to inform the InboxPresenter
so that the InboxView
would display the mail as "favorite", right? In that case, it doesn't is up to the presenters to talk to each other. If you have look on the "model layer" of MailPresenter
and InboxPresenter
you will see that both have the same "model" MailProvider
. So both presenter should observe the MailProvider
for changes. If MailPresenter
updates a mail (mark as favorite) thenMailPresenter
will call MailProvider.markMailAsFavorite(mail)
. Since InboxPresenter
is observing the MailProvider
as well, MailProvider
have to notify all its observers incl. InboxPresenter
and then the InboxPresenter
will update the InboxView
. So there is no direct communication between MailPresenter
and InboxPresenter
: The "communication" is
MailPresenter --markAsStarred()--> MailProvider --updates--> InboxPresenter
So basically this is the observer pattern. People tend to use an EventBus to communicate such "mail marked as starred" Events between MailPresenter
and InboxPresenter
, but this is just because they are too lazy to implement the observer pattern on MailProvider
correctly.
Is that what you mean with "presenter communication"?
HeaderPresenter
and ListPresenter
talk to the same proxy instance as "model layer". Then it's the responsibility of the proxy to ensure that both get the same result at once (similar to http client batched requests). With RxJava you could use share()
operator. Both Presenter subscribe to this "shared" observable. By doing so its ensured that both get the same result at the same time in their onNext()
.Hope that answers your question.
btw. as some people asked me about PresentationModel
in combination with RecyclerView (I have mentioned that at the end where we have talked about testing), I have written those things down in a blog post:
http://hannesdorfmann.com/android/adapter-commands
@sockeqwe with gmail, if MailView button causing change a fragment, which should be done from activity or something. How will you do this operation? Activity scoped Navigator? What's your opinion in general: activity scoped presenters vs singleton scoped (easier to maintain view state in case of orientation change)
Damn, I had navigation on list of topics to talk about during the podcast, but I forgot about that and we ran out of time anyway @artem-zinnatullin
I don't think that Navigation is the responsibility of the Presenter. I think Navigation should be part of the "View" layer. That doesn't mean that we hardcode and couple everything in the View i.e. Fragment. What I typically have a Navigator
interface / class.
So let's say there is the GmailActivity
that displays InboxFragment
(InboxView) and MailFragment
(MailView). So the Activity is just a "container" and therefore doesn't need a presenter. So now let's say that clicking on a "Mail Item" from InboxFragment
should display the "mail details" in MailFragment
(or replace the old one if there is already one).
I have some kind of Navigation component (let's call this Navigator
) in InboxFragment (i.e. injected by dagger and scoped to GmaiActivity
). So when clicking on a Mail Item and want to display this mail then I would call in Fragment (not in Presenter) Navigator.showMail(mailID)
(please note that i don't name it showMailFragment(mailId), we don't want to specify some implementation details). So Navigator.showMail(mailId)
is now responsible to bring up MailFragment
. So Navigator could internally have knowledge of FragmentManager and would start and commit the fragment transaction (Navigator will be Activity scoped with dagger).
i.e.
class TabletNavigator implements Navigator {
private Activity activity;
public TabletNavigator(Activity activity){
this.activity = activity;
}
@Override
public void showMail(int mailId){
MailFragment fragment = ...;
activity.getSupportFragmentManager()
.beginTransaction()
.replace(R.id.detailsContainer, fragment)
.commit();
}
...
}
and then I could also have a PhoneNavigator
that starts an Intent to bring up an activity that is just a container for MailFragment
:
class TabletNavigator implements Navigator {
private Activity activity;
public TabletNavigator(Activity activity){
this.activity = activity;
}
@Override
public void showMail(int mailId){
Intent i = new Intent(activity, MailDetailsActivity.class);
activity.startActivity(i);
}
...
}
And then in the InboxFragment
I would just inject the Navigator
via dagger and depending if the user uses a Tablet or Phone:
class InboxFragment extends Fragment {
@Inject Navigator navigator;
void onMailClicked(Mail mail){
navigator.showMail(mail.getId());
}
I also think that this concept of Navigator plays quite good with shared Element transitions. We could have a method like Navigator.showMail(int mailId, View sharedElement)
which then will setup the shared FragmentTransition or ActivityTransition.
Probably @artem-zinnatullin is doing that entirely different or have found a better way.
activity scoped presenters vs singleton scoped
Not sure what exactly do you mean with scoped presenters ?
Scoping depends on the complexity of your app. i.e. if I have one single Retrofit
object in my whole app than I do singleton scoped. I would say that I start singleton scoped and most of my apps are fine with that. If during development I found out that some components are not always needed or lifecycle bounded (i.e. Navigator will be activity scoped, because it has a hard reference to the activity) then I will begin to create smaller scoped dagger components.
Typically I have businesslogic singleton scoped (app wide) and Navigation activity scoped.
Thank you for clarification navigation. About presenter's scope. Mosby used per activity presenter, instead of singleton scoped presenter. Why? It is easier to maintain view state in singleton, and there is no problems, if singleton doesn't know anything of view internals.
First, please note that Mosby is just some kind of scaffold and you can plug in custom behaviour very easily by providing a custom delegate (see http://hannesdorfmann.com/mosby/viewstate/ scroll down until "Delegation chapter"), so you could add your custom "singleton presenter delegate" very easily.
So Mosby's "default behaviour" is to have presenters that live as long as the view lives (and I don't see any reason why the presenter should live longer than the corresponding view, presenter will be garbage collected when the view gets garbage collected). However, this "default behaviour" is smart enough to determine if the view will be destroyed permanently\ (i.e. user has pressed back button) or has been destroyed temporarly because of a screen orientation changes and will be recreated afterwards. With the later one, the presenter can survive screen orientation changes and the "landscape view" gets simply attached / detached from the same presenter instance as "portrait view".
I believe that this "default behaviour" makes sense for 95% percent of an apps use case.
So what about "singletons presenters". Well, when will they garbage collected? Never. What if you have a view that is uses multiple times, i.e. You have Fragment (View) and use this Fragment in different Activities. Will they all share the same singleton presenter instance? Does that makes sense? Or what if you reuse the same Fragment in a ViewPager?
I don't see any advantage of implementing a presenter as "singleton" for this reasons. Have I missed something? I would like to hear what the advantages of "singleton" presenters are compared to "Mosby's" default behaviour.
Great start, guys!
How do you organize package tree with mvp?
@b1uebyte usually I group my classes by feature like here
Basically I group every screen (or subscreen like a fragment or a custom view) in is own package containing all the related classes except model classes which live in is own package like this (MVP example):
Typically I have a java gradle module for the "model" layer. So I move the "model package" from above into is own java gradle module.
Probably @artem-zinnatullin is doing that entirely different or have found a better way.
Router is still kind of "idk" thing, I tried several approaches and hadn't found clear solution yet, last one was just a class that has methods like openFeedbackScreen()
and only presenter knows about it.
How do you organize package tree with mvp?
Here I'm opposite to @sockeqwe and prefer packaging by components
This is a very cool Podcast keep them coming found you guys on Android Dev Digest
@sockeqwe can we apply similar pattern like Navigator for NavigationDrawer i.e. when user selects different item from the drawer .
@sockeqwe also do you have any good example of Repository Pattern
So I started to answer here on github your question about the Repository Pattern. It turned out that my answer is getting too long, so I decided to move my answer in a dedicated blog post: http://hannesdorfmann.com/android/evolution-of-the-repository-pattern
A good example (also mentioned in the blog post) can be found here: https://github.com/android10/Android-CleanArchitecture
@sockeqwe do you use navigator pattern like thing for Navigation Drawer
I don't think so because I don't even know what the Navigator Pattern is ๐
I mean, I do have classes called Navigator
, but I'm not sure if this is the Navigator Pattern you are talking about.
I.e. I have a interface Navigator
for a imaginary News Magazine app:
interface Navigator {
void showNewsList();
void showNewsDetails(int newsId);
}
The I would have a PhoneNavigator:
class PhoneNavigator implements Navigator {
private Activity activity;
@Override
void showNewsDetails(int newsId){
Intent intent = new Intent( activity, NewsDetailsActivity.class);
intent.putExtra("NewsId", newsId);
activity.startActivity(intent);
}
...
}
NewsDetailsActivity basically hosts a NewsDetailsFragment
.
Then I also have a TabletNavigator
class TabletNavigator implements Navigator {
private Activity activity;
@Override
void showNewsDetails(int newsId){
activity.getFragmentManager()
.beginTransaction()
.replace(R.id.container, NewsDetailsFragment.newInstance(newsId))
.commit();
}
...
}
The difference of both navigators is how they display "navigate" in the UI. PhoneNavigator starts a new Activity to display a list of news while TabletNavigator switches a Fragment in a Master / Detail view in the same Activity.
And then in my activities / fragments I instantiate either a PhoneNavigator or TabletNavigator (or inject one via dagger) depending on if the device is a Phone or a Tablet. By clicking on a view somewhere in the UI I will then call navigator.showNewsDetails(newsId)
and I don't care about the implementation details (like starting Activity or replacing a Fragment). It's only about hiding implementation details and separation of concerns / single responsibility.
Is that what you call the "Navigator Pattern"?
Thanks that the same thing I was talking about @sockeqwe . I have one more question for you in the podcast you said that we can have more than one Presenter in our Activity for example in this image
the Inbox View has two presenter the Fragment presenter and SearchViewPresenter so my biggest concern is how will I attach View to the Presenter cause in my app I have a function setView(ViewInterface interface)
which attach my view to presenter but how can I do same with some other View(Widget)
Use View.onAttachedToWindow() and View.onDetachFromWindow()
@sockeqwe I've a question concerning your comment of using AdapterCommands
or DiffUtil
in the presenter. (I love the idea but I'm having an issue implementing it. Doing it in the adapter on the main thread is causing issues ~10ms calculation) I've read your blogposts about MVI, and I absolutely loved them!
I'm currently implementing it in a hobby project and I am using a BehaviorRelay
to cache the latest "ViewModelState".
My questions is if I reduced the state of my previous List<Data>
and my new List<Data>
with a resulting List<AdapterCommands>
I don't want that List<AdapterCommands>
on a rotation change because in case I'm using a Fragment/Controller my adapter would be retained (same lifecycle as Fragment/Controller). The adapter commands would make my adapter out of sync.
Do you have a good solution for this? I can currently only think of dirty onces.
Awesome podcast btw ๐
The "clean" way would be to let the information that you have applied List<AdapterCommands>
or DiffResult
flow down to the state reducer. So basically its just yet another intent. The State Reducer then would create a new "ViewModelState" with an empty list in case of AdapterCommands
or an empty DiffResult
since "nothing" has changed in the data list. Then render(newState)
will be called again, but since List<AdapterCommands>
/ DiffResult
is empty no ViewHolder will be updated, so no UI widget will be updated at all, but you are in a clean new state. Then after a screen orientation change you would apply exactly the previous model with the empty List<AdapterCommands>
/ DiffResult
Other workarounds that may or may not work in your use case:
List<AdapterCommands>
/ DiffResult
if RecyclerView's) Adapter's dataset != null. That works for screen orientation changes and most likely for back stack navigation (so it works whenever the view is destroyed).
MviFragment.isRestoringViewState()
that can be used inside your view.render(ViewModelState)
to determine whether or not the render()
method is called because the view gets reattached (i.e. screen orientation change, back stack navigation etc.) or not. You could implement something like that for your app too.Go the clean way or use work around? It depends how complex your app / state is and if you are a "purist" or not :)
hope that helps!
@sockeqwe thanks for the quick response. I took a look at Mosby MVI
and Mosby Conductor MVI
but I have yet to give it a try.
I thought of "clearing" the List<AdapterCommands>
when detach the view from the presenter, in a similar way as you described just at a different point in time. My presenters are retained on config change, and are singletons for each view. So no conflict can occur in cached view states in the behavior relay.
dataset != null
that strategy won't work for me since my adapter is also retained on config change, I'm using Conductor and it scoped to the life of the Controller with Dagger. Either way I'm skeptical about last 2 workarounds since.
Thanks for the response! It was more of a general wonderment I had when listening to the podcast. I'm currently doing it similarly to this https://github.com/pakoito/FunctionalAndroidReference/blob/80dd1a6d26647d3023ae234f0b7b76422eecd9b5/app/src/main/java/com/pacoworks/dereference/widgets/BaseRecyclerAdapter.java but I want to move it away from the adapter / ui thread.
Just a reminder: Retaining the Adapter on orientation changes is a memory leak since ViewHolders and their views has been instantiated with the layout inflater from the very first activity. So that activity cant be garbage collected.
Simon Vergauwen notifications@github.com schrieb am Di., 28. Mรคrz 2017, 19:13:
@sockeqwe https://github.com/sockeqwe thanks for the quick response. I took a look at Mosby MVI and Mosby Conductor MVI but I have yet to give it a try.
I thought of "clearing" the List
when detach the view from the presenter, in a similar way as you described just at a different point in time. My presenters are retained on config change, and are singletons for each view. So no conflict can occur in cached view states in the behavior relay. dataset != null that strategy won't work for me since my adapter is also retained on config change, I'm using Conductor and it scoped to the life of the Controller with Dagger. Either way I'm skeptical about last 2 workarounds since.
Thanks for the response! It was more of a general wonderment I had when listening to the podcast. I'm currently doing it similarly to this https://github.com/pakoito/FunctionalAndroidReference/blob/80dd1a6d26647d3023ae234f0b7b76422eecd9b5/app/src/main/java/com/pacoworks/dereference/widgets/BaseRecyclerAdapter.java but I want to move it away from the adapter / ui thread.
โ You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/artem-zinnatullin/TheContext-Podcast/issues/1#issuecomment-289839795, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjnrrqyxfaiNGlhtMq6biFPnaRpiIGtks5rqT-qgaJpZM4HUABV .
@sockeqwe thanks for the heads up. I was unaware that an Adapter kept actual references to the viewholders. I thought it was more like a sort of factory/populator.
Hey @artem-zinnatullin @sockeqwe that is awesome podcast episode. and It clear so many things about MVP. It's really helpful if you add some good android MVP demo link ?
It's 2.5 years old episode tho ๐ธ
Afaik all hosts of the podcast moved past MVP since then, we want to record an episode about MVI / Reactive Redux sometime in future, but no ETA
Yay! Feedback is really appreciated :smile_cat: