konmik / nucleus

Nucleus is an Android library, which utilizes the Model-View-Presenter pattern to properly connect background tasks with visual parts of an application.
MIT License
1.98k stars 253 forks source link

Question regarding getView() and reference the View in the presenter. #145

Closed Orbyt closed 6 years ago

Orbyt commented 7 years ago

This is simply a question, please label it as such.

I understand getView() is deprecated. The docs comments on the method mention the following:

/**
 * Please, use restartableXX and deliverXX methods for pushing data from RxPresenter into View.
 */

What if an Android Context is needed, in cases like parsing a local .json file from assets?

For example:

InputStream is = getView().getAssets().open("data.json");

What is the best practice for this particular case?

TWiStErRob commented 7 years ago

If you need "a" Context, I think it's best to dependency inject the application context into your presenter or abstract that dependency away so you'll get a stream from "somewhere".

Orbyt commented 7 years ago

We're explicitly avoiding dependency injection for this project. I was considering reading the file into a String in the Activity and then just passing the presenter the JSON String, but wanted to determine any alternatives.

konmik commented 7 years ago

App.getInstance() ?

Orbyt commented 7 years ago

This is quite strange, and hopefully related. I had been using .getView() as mentioned, and it worked just fine, and this question was posted as a matter of determining best practices.

However, last Friday a colleague discovered an issue with the application when running in API 25. I found it strange that I had not seen the issue, but after creating a new emulator and installing API 25, I was able to recreate it. I then switched back to the emulator I had been using for development (running API 23), and reinstalled the application. Suddenly, the above described issue appeared. After debugging the issue last Friday and today, it appeared to be a conflict between Firebase and a support library. I found it strange that the issue had suddenly appeared, and that I had not seen it before. Even old revisions that had been working, suddenly were having the same issue.

Fast forward a bit, and I seemed to have resolved the issue described above. However, for some reason, the presenter described in this original issue is no longer functioning as it was. Two issues appear to be present. getView() occasionally returns null, and the restartableLatestCache does not seem to be getting a response in some cases. Below is most of the code for the presenter:

/**
 * Presenter for the SplashActivity.
 * This presenter will parse local data files and signal it's view when completed.
 */
public class SplashActivityPresenter extends RxPresenter<SplashActivity> {

    private static final String TAG = "SplashActivityPresenter";

    /**
     * Request identifier used when parsing the local .json files is needed.
     */
    public static final int REQUEST = 1;

    /**
     * Request identifier used when the local .json files have already been parsed
     * and the Realm DB has been populated.
     */
    public static final int REQUEST_ALREADY_LOADED = 2;

    private Context mContext;

    @Override
    protected void onCreate(Bundle savedState) {
        super.onCreate(savedState);
        restartableLatestCache(REQUEST,
                () -> getData().subscribeOn(Schedulers.io()).observeOn(AndroidSchedulers.mainThread()),
                (view, response) -> {
                    Log.d(TAG, "onResponse: request");
                    view.onData();
                },
                (view, err) -> {
                    Log.d(TAG, "onError: " + err.getMessage());
                    view.onError();
                });

        restartableLatestCache(REQUEST_ALREADY_LOADED,
                () -> Observable.just(0).subscribeOn(AndroidSchedulers.mainThread()).observeOn(AndroidSchedulers.mainThread()),
                (view3, response) -> {
                    Log.d(TAG, "onResponse: request already loaded");
                    view3.onData();
                },
                (view, err) -> {
                    Log.d(TAG, "onError: " + err.getMessage());
                    view.onError();
                });
    }

    /**
     * Begin a request.
     */
    public void request(Context context) {
        mContext = context;
        Realm.getDefaultInstance().executeTransaction(realm -> {
            Bundle bundle = realm.where(Bundle.class).findFirst();
            if (bundle != null) {
                start(REQUEST_ALREADY_LOADED);
            } else {
                start(REQUEST);
            }
        });
    }

    /**
     * Creates an observable to parse the local .json files.
     * @return An Observable which will call it's subscriber's onComplete()
     * once parsing has been completed.
     */
    private Observable getData() {
        return Observable.create(subscriber -> {
            parseBundles();
            parsePhrases();
            Log.d(TAG, "getData: onComplete is about to be called.");
            subscriber.onComplete();
            Log.d(TAG, "getData: onComplete has been called");
        });
    }

    /**
     * Parses file and saves the data to Realm.
     */
    private void parsePhrases() {

        String msg = getView() == null ? "The view is null" : "The view isn't null";
        Log.d(TAG, "parsePhrases(): " + msg);
        String json = null;
        try {
            InputStream is = mContext.getAssets().open("someData.json");
            int size = is.available();
            byte[] buffer = new byte[size];
            is.read(buffer);
            is.close();
            json = new String(buffer, "UTF-8");
        } catch (IOException ex) {
            ex.printStackTrace();
            return;
        }
        JSONArray array = null;
        try {
            array = new JSONArray(json);
        } catch (JSONException e) {
            e.printStackTrace();
        }

        if (array != null) {
            for (int i = 0; i < array.length(); i++) {
                Phrase phrase = new Phrase();

                try {
                    phrase.setVideoPopupInfo(obj.getString("videoPopupInfo"));
                } catch (JSONException e) {
                    Log.e(TAG, "parsePhrases: " +  e.getMessage() );
                }

                //..A note, the above code will often throw and catch the JSONException, but this is ignored as certain JSON objects in the file don't have this property.

                Realm realm = Realm.getDefaultInstance();
                realm.beginTransaction();
                realm.copyToRealm(phrase);
                realm.commitTransaction();
            }
        }
    }

    /**
     * Parses the file and saves the data to Realm.
     */
    private void parseBundles() {
        String msg = getView() == null ? "The view is null" : "The view isn't null";
        Log.d(TAG, "parseBundles(): " + msg);
        String json = null;
        try {
            InputStream is = mContext.getAssets().open("someDataTwo.json");
            int size = is.available();
            byte[] buffer = new byte[size];
            is.read(buffer);
            is.close();
            json = new String(buffer, "UTF-8");
        } catch (IOException ex) {
            ex.printStackTrace();
        }

        JSONArray array = null;
        try {
            array = new JSONArray(json);
        } catch (JSONException e) {
            e.printStackTrace();
        }

        if (array != null) {

            for (int i = 0; i < array.length(); i++) {

                Bundle bundle = new Bundle();
                JSONObject obj = null;

                try {
                    obj = array.getJSONObject(i);
                } catch (JSONException e) {
                    e.printStackTrace();
                }

                //..Parse a number of JSON properties and save to bundle object.

                Realm realm = Realm.getDefaultInstance();
                realm.beginTransaction();
                realm.copyToRealm(bundle);
                realm.commitTransaction();
            }
        }
    }
}

getPresenter().request() is being called in the Activity's onCreate(). Moving the call to onResume() does seem to alleviate the issue of the view being null, however as most example's show this type of call being made in onCreate(), i'm curious as to what the best option is for this particular case.

Regardless, the issue of the response not being delivered occurs in all cases. As you can see, getData() creates and returns an observable. It calls the two parsing methods, and then calls subscriber.onComplete(). I can confirm this due to the two log statements placed before it. However, the restartableLatestCache for the REQUEST ID never gets the response. The call to view.onData() does not occur.

What is the issue here, and how can it be resolved? If this question is better posted as a separate issue, please let me know.

konmik commented 7 years ago

Can it be because of RxJava version update? Or support library? Support library sometimes break lifecycles.

Orbyt commented 7 years ago

@konmik I have all dependencies declared with explicit versions.

RxJava is compile 'io.reactivex.rxjava2:rxandroid:2.0.1', support libraries are all 25.3.1. I have not changed any of these recently.

Something interesting I've noticed: the restartableLatestCache for the REQUEST_ALREADY_LOADED seems to work as expected. If the Realm database is populated, start(REQUEST_ALREADY_LOADED) is called, and the call to view.onData() there goes through just fine.

Orbyt commented 7 years ago

I expanded a few of the lambdas into anonymous inner classes:

restartableFirst(REQUEST,
                new Factory<Observable<Object>>() {
                    @Override
                    public Observable<Object> create() {
                        return SplashActivityPresenter.this.getData().subscribeOn(Schedulers.io()).observeOn(AndroidSchedulers.mainThread());
                    }
                },
                new BiConsumer<SplashActivity, Object>() {
                    @Override
                    public void accept(SplashActivity view, Object response) throws Exception {
                        Log.d(TAG, "onResponse: request");
                        //view.onData();
                    }
                },
                new BiConsumer<SplashActivity, Throwable>() {
                    @Override
                    public void accept(SplashActivity view, Throwable err) throws Exception {
                        Log.d(TAG, "onError: " + err.getMessage());
                        view.onError();
                    }
                });

The following line is highlighted with a warning:

return SplashActivityPresenter.this.getData().subscribeOn(Schedulers.io()).observeOn(AndroidSchedulers.mainThread());

Unchecked assignment: 'io.reactivex.Observable' to 'io.reactivex.Observable'. Reason: 'SplashActivityPresenter.this.getData().subscribeOn(Schedulers.io())' has raw type, so result of observeOn is erased

I suppose that may be the issue? Currently, getData() returns a plain Observable. Do I need to make it return some type, eg. Observable<T>?

Orbyt commented 7 years ago

I made the following changes:

I modified the below method:

private Observable getData() {
        return Observable.create(subscriber -> {
            parseBundles();
            parsePhrases();
            Log.d(TAG, "getData: " + "onComplete about to be called.");
            subscriber.onComplete();
            Log.d(TAG, "getData: onComplete has been called");
        });
}

to:

 private Observable<Object> getData() {
        return Observable.fromCallable(new Callable<Object>() {
            @Override
            public Object call() throws Exception {
                parseBundles();
                parsePhrases();
                return 0;
            }
        });
}

This appears to work. Why might the implementation using Observable.create() not work as expected?

TWiStErRob commented 7 years ago

@Orbyt https://artemzin.com/blog/rxjava-defer-execution-of-function-via-fromcallable/ ? (search for others as well). There may be some magic in fromCallable that helps to make it work.

I suggest you also look into Completable / Single alternatives to 0 and 1 emission Observables, they make code intentions much cleaner.

Orbyt commented 7 years ago

@TWiStErRob I have previously come across that page when looking at alternatives to .create(). Though, I'm still curious as to why .fromCallable() works in this case.

The reason for returning a 0 was simply return something, the value is not used.

konmik commented 6 years ago

I need more data on the issue, preferable a minimal repo with reproduced error. Is looks like yet another support library bug for me (I remember they broke view lifecycle inside fragment one day, so this also can be the reason).