[Question] Where is the best place to start loading data and persist it?
Closed this issue · 8 comments
I am currently investigating different MVxx libraries and approaches on Android and stumbled upon this one. As a show case for each library I am creating simple list+detail (image gallery) app using REST services as data provider.
Lets assume we are using one Activity (MainActivity) and two fragments (GalleryFragment, DetailsFragment) in our app. Implementation of activity is quite straightforward as it requires no view or presenter - is simply starts and manages fragments.
Now, we need to implement GalleryFragment which needs to display a list of images.
So we need a view:
public interface GalleryView extends TiView {
void startLoading(boolean pullToRefresh);
void stopLoading();
void showError(Throwable error);
void showGallery(List<Image> images);
}
We need layout for fragment:
<?xml version="1.0" encoding="utf-8"?>
<FrameLayout
xmlns:android="http://schemas.android.com/apk/res/android"
xmlns:tools="http://schemas.android.com/tools"
android:layout_width="match_parent"
android:layout_height="match_parent"
android:layout_marginLeft="16dp"
android:layout_marginRight="16dp">
<ProgressBar
android:id="@+id/progress"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:layout_gravity="center"
android:indeterminate="true"/>
<android.support.v4.widget.SwipeRefreshLayout
android:id="@+id/contentView"
android:layout_width="match_parent"
android:layout_height="match_parent">
<android.support.v7.widget.RecyclerView
android:id="@+id/recyclerView"
android:name="pl.fzymek.tiimagegallery.gallery.GalleryFragment"
android:layout_width="match_parent"
android:layout_height="match_parent"
tools:context="pl.fzymek.tiimagegallery.gallery.GalleryFragment"
tools:listitem="@layout/fragment_gallery_item"/>
</android.support.v4.widget.SwipeRefreshLayout>
<TextView
tools:text="Error happened! :("
android:id="@+id/error"
android:layout_gravity="center"
android:layout_width="wrap_content"
android:layout_height="wrap_content"/>
</FrameLayout>
And we need a Fragment:
public class GalleryFragment extends TiFragment<GalleryPresenter, GalleryView> implements GalleryView, SwipeRefreshLayout.OnRefreshListener {
@BindView(R.id.recyclerView)
RecyclerView recyclerView;
@BindView(R.id.progress)
ProgressBar progressBar;
@BindView(R.id.error)
TextView error;
@BindView(R.id.contentView)
SwipeRefreshLayout contentView;
GalleryAdapter adapter;
Unbinder unbinder;
@Override
public View onCreateView(LayoutInflater inflater, ViewGroup container,
Bundle savedInstanceState) {
return inflater.inflate(R.layout.fragment_gallery, container, false);
}
@Override
public void onViewCreated(View view, Bundle savedInstanceState) {
unbinder = ButterKnife.bind(this, view);
contentView.setOnRefreshListener(this);
Context context = view.getContext();
if (getResources().getConfiguration().orientation == Configuration.ORIENTATION_PORTRAIT) {
recyclerView.setLayoutManager(new LinearLayoutManager(context));
} else {
recyclerView.setLayoutManager(new GridLayoutManager(context, 3));
}
adapter = new GalleryAdapter();
recyclerView.addItemDecoration(new SpaceDecoration());
recyclerView.setAdapter(adapter);
}
@Override
public void onDestroyView() {
super.onDestroyView();
unbinder.unbind();
}
@NonNull
@Override
public GalleryPresenter providePresenter() {
Timber.d("providePresenter");
return new GalleryPresenter();
}
@Override
public void startLoading(boolean pullToRefresh) {
progressBar.setVisibility(View.VISIBLE);
contentView.setVisibility(View.GONE);
error.setVisibility(View.GONE);
contentView.setRefreshing(pullToRefresh);
}
@Override
public void stopLoading() {
contentView.setVisibility(View.VISIBLE);
progressBar.setVisibility(View.GONE);
error.setVisibility(View.GONE);
contentView.setRefreshing(false);
}
@Override
public void showError(Throwable err) {
error.setVisibility(View.VISIBLE);
contentView.setVisibility(View.GONE);
progressBar.setVisibility(View.GONE);
contentView.setRefreshing(false);
}
@Override
public void showGallery(List<Image> images) {
adapter.setData(images);
}
@Override
public void onRefresh() {
loadData(true);
}
private void loadData(boolean pullToRefresh) {
getPresenter().loadData(pullToRefresh);
}
}
And finally, we also have our presenter:
class GalleryPresenter extends TiPresenter<GalleryView> {
private final static TiConfiguration PRESENTER_CONFIGURATION = new TiConfiguration.Builder()
.setRetainPresenterEnabled(true)
.build();
private GettyImagesService service;
GalleryPresenter() {
super(PRESENTER_CONFIGURATION);
Retrofit retrofit = new Retrofit.Builder()
.addCallAdapterFactory(RxJavaCallAdapterFactory.create())
.addConverterFactory(GsonConverterFactory.create())
.baseUrl(Config.GETTYIMAGES_API_URL)
.build();
service = retrofit.create(GettyImagesService.class);
}
@Override
protected void onCreate() {
super.onCreate();
Timber.d("onCreate");
}
@Override
protected void onDestroy() {
super.onDestroy();
Timber.d("onDestroy");
}
@Override
protected void onSleep() {
super.onSleep();
Timber.d("onSleep");
}
@Override
protected void onWakeUp() {
super.onWakeUp();
Timber.d("onWakeUp");
}
public void loadData(String phrase, boolean pullToRefresh) {
getView().startLoading(pullToRefresh);
Timber.d("loadData %s", phrase);
service.getImages(phrase)
.subscribeOn(Schedulers.newThread())
.observeOn(AndroidSchedulers.mainThread())
.subscribe(
result -> {
getView().showGallery(result.getImages());
getView().stopLoading();
},
error -> {
getView().showError(error);
}
);
}
}
Simple as that.
What I do not understand, is when should I call loadData() to trigger a network request, and how and when to persist retrieved data between screen orientation changes.
Usually we'd start network request somewhere in onViewCreated() or onCreateView method of fragment as in that moment we have our view ready. Unfortunately, when using ThirtyInch, this in not true, as view is binded to presenter in onStart() method and calling GalleryPresenter#loadData() throws NPE as getView() returns null.
Solution to this would be to defer it a little and load data in onStart() method of fragment or onWakeUp() method of presenter (as presented in sample, but when using onWakeUp() I cannot pass my constructed query to the presenter). This works like a charm, but...
Since I am retaining a presenter, I see a problem with this solution when rotating a screen while network request is pending. Flow is in this case:
- Fragment/View and Presenter gets created
- Fragment is started (
onStart) and Presenter is woken uponWakeUp-> trigger request 1 - Screen gets rotated
- Fragment/View is recreated but Presenter is retained (ok)
- Fragment is started (
onStart) and Presenter is woken uponWakeUp-> trigger request 2 - Request 1 finishes -> view shows data 1
- Rotate screen again
- We go back here to recreating view, retaining presenter and triggering another request(no 3)
- Now we have view displaying progress, although data was fetched (request 1) and displayed for a while and 2 pending requests which will trigger new view updates
What I don't understand here is, how to correctly request data and persist it using ThirtyInch - is the persistence handled by framework (setRetainPresenterEnabled(true) ?) or should I rely on androids onSaveInstanceState(). Not retaining a presenter would leave me nothing more that Mosby (ad described here). So what should I do in order to use all features of TI?
I've been able to find a solution to this problem, but is still feels like I am not using the library in a good way. I am calling loadData() from GalleryFragment#onResume() and I needed to change my presenter so it caches data/error and looks like this:
class GalleryPresenter extends TiPresenter<GalleryView> {
private final static TiConfiguration PRESENTER_CONFIGURATION = new TiConfiguration.Builder()
.setRetainPresenterEnabled(true)
.build();
private GettyImagesService service;
private GettySearchResult result;
private Throwable error;
private boolean isRequestPending = false;
GalleryPresenter() {
super(PRESENTER_CONFIGURATION);
Retrofit retrofit = new Retrofit.Builder()
.addCallAdapterFactory(RxJavaCallAdapterFactory.create())
.addConverterFactory(GsonConverterFactory.create())
.baseUrl(Config.GETTYIMAGES_API_URL)
.build();
service = retrofit.create(GettyImagesService.class);
}
@Override
protected void onCreate() {
super.onCreate();
Timber.d("onCreate");
}
@Override
protected void onDestroy() {
super.onDestroy();
Timber.d("onDestroy");
}
@Override
protected void onSleep() {
super.onSleep();
Timber.d("onSleep");
}
@Override
protected void onWakeUp() {
super.onWakeUp();
Timber.d("onWakeUp");
updateView();
}
public void loadData(String phrase, boolean pullToRefresh) {
if (pullToRefresh) {
clearData();
}
if (!hasData() && !isRequestPending) {
Timber.d("loadData %s", phrase);
getView().startLoading(pullToRefresh);
isRequestPending = true;
service.getImages(phrase)
.subscribeOn(Schedulers.newThread())
.observeOn(AndroidSchedulers.mainThread())
.subscribe(
result -> {
isRequestPending = false;
GalleryPresenter.this.result = result;
updateView();
},
error -> {
isRequestPending = false;
GalleryPresenter.this.error = error;
updateView();
}
);
}
}
private void clearData() {
error = null;
result = null;
}
private boolean hasData() {
boolean hasData = error != null || result != null;
Timber.d("hasData %b", hasData);
return hasData;
}
private void updateView() {
Timber.d("updateView");
if (isAwake()) {
if (error != null) {
Timber.d("showError");
getView().showError(error);
}
if (result != null) {
Timber.d("showGallery");
getView().showGallery(result.getImages());
getView().stopLoading();
}
}
}
}
Please push this sample project somewhere so I'm able to pivot your code and reference parts of my solution in my answer
@passsy You can find demo under https://github.com/fzymek/MVP-Sample
Hey Filip, finally found the time to update your sample project the ThirtyInch-way. Here is the updated code: https://github.com/passsy/MVP-Sample/blob/master/app/src/main/java/pl/fzymek/simplegallery/gallery/GalleryPresenter.java
Let me explain what I did:
First of all you shouldn't trigger loading from the Fragment (View). Never call something like loadData() from the View. The view should send events, and not tell the presenter what to do. Basically in your case there are two events when the data should be loaded. Initially and after a pull to refresh. The initial loading can be triggered by TiPresenter#onCreate() and the pull to refresh event can be simply forwarded to the presenter (presenter.onRefresh()).
Both events trigger the loadData() method and udpate the loading state as well as the error and result information.
You already implemented a updateView() method which generally updates the view based on the data known in the presenter. I modified it a bit but the idea was correct. Additionally I added @DistinctUntilChanged to the showGallery(list) and showLoadingIndicator(bool) method which prevents too many similar calls to the UI.
Cancelling the running request is the last missing piece. I cancel it it onDestroy() in case the request isn't finished when the user exits the Activity. It continues loading the data over a configuration change. When the request returns while the view isn't attached all state is known by the presenter. Although updateView() was called it never updates the view due to the null check. When the Activity resumes the view gets attached and onWakeUp() gets called triggering updateView(). This updates the view with the new data from the request.
Never call something like loadData() from the View. The view should send events, and not tell the presenter what to do.
Can you elaborate on the reasons why we should follow this rule ? Because I can read the exact opposite of what you've said on several blog posts including this valuable one.
I don't see the relationship to PRESENTERS DON'T NEED LIFECYCLE EVENTS.
Let's say it this way: There are good and bad events which can be forwarded to the presenter. Lifecycle events are the bad ones. They don't express a user interaction. Don't tell the presenter what to do, tell what happened. presenter.onButtonClicked(): Unit instead of presenter.loadData(): SomeData
Here is the good data flow which works in 99.8% of all cases:
View -----(User event)---> Presenter
View <--(Data, Actions)--- Presenter
Keep in mind that calling the Presenter from the View should not return data. The presenter should decide when and if view should get data or when actions should be executed.
The relationship with the article is that you seem to recommand to have lifecycle event in the presenter
The initial loading can be triggered by TiPresenter#onCreate()
And something like
Never call something like loadData() from the View.
Seem to be the opposite of what the article says. I think the best way is to have a loadData() method in the presenter and to trigger it on any Android lifecycle event we would like to (and which makes sense of course).
I'm ok with you when you say that the presenter must no return data and should decide when to forward it to view.
TiPresenter#onCreate is not the same as Activity#onCreate. TiPresenter#onCreate is only called once when the Presenter is created. After a configuration change, when Activity#onCreate with savedInstanceState != null is called the TiPresenter#onCreate will not be trigger. Same for TiPresenter#onDestroy which is only called once when the Activity will be finished, not always when Activity#onDestroy will be called.