benoitletondor / Android-Studio-MVP-template

Android MVP template for Android Studio
Apache License 2.0
627 stars 121 forks source link

Crash on Support library 27.1.0 #14

Closed isamotiuc closed 6 years ago

isamotiuc commented 6 years ago

In new Support library 27.1.0 was made some changed with loaders, this led to some issues with this approach. For example, presenter is null in Activity in onStart() method.

benoitletondor commented 6 years ago

Hi @isamotiuc !

Thanks for reporting this issue. I'll investigate this tomorrow.

I indeed saw that Google changed Loader implementation to use LifeCycle but they didn't say it should change anything in the behavior so I'll check what's going on.

I'll keep you posted.

PS: If you respect the MVP pattern, you have little to no reason to call your presenter directly from onStart though ;)

benoitletondor commented 6 years ago

Hi again @isamotiuc,

So I did a little check and indeed the implementation made by Google actually changes the runtime of the Loader and now it finishes to load after your Activity 's onStart method is call.

But (because there is a but), you should not call your presenter from the onStart method of your activity, since the presenter's onStart(boolean viewCreated) method will be called when the load is finish and that's where you're supposed to do the work.

This case (if the presenter is not ready when onStart is called), is actually handled by the MVP Framework so everything works ok, even with this change. So the main issue here is that your implementation is not correct and actually breaks the MVP pattern.

Let me know if you need more help.

Meanwhile, I'll add a note to the README to be sure that it's clearer than it is right now.

Thank you for reporting.

benoitletondor commented 6 years ago

Just a note that Google is aware of the issue and it will be fixed on a later release: https://issuetracker.google.com/issues/74225064

isamotiuc commented 6 years ago

@benoitletondor Thank you for your effort. I set a page listener in onStart method and handle it on presenter.

 viewPager.addOnPageChangeListener(mPresenter.getPagerListener());

May be better will be to do something like this. 🤔

viewPager.addOnPageChangeListener(new ViewPager.OnPageChangeListener() {
            @Override
            public void onPageScrolled(int position, float positionOffset, int positionOffsetPixels) {
                mPresenter.onPageScrolled()
            }

            @Override
            public void onPageSelected(int position) {

            }

            @Override
            public void onPageScrollStateChanged(int state) {

            }
        });

WDYT?

benoitletondor commented 6 years ago

Hi @isamotiuc !

Sorry for the delay, totally forgot to answer! My bad!

Yes, your second approach is better imo because it separates more the view and the presenter part. The view should only be calling the presenter for user input events. So basically it would be something like you did, but with a test of presenter nullity:

viewPager.addOnPageChangeListener(new ViewPager.OnPageChangeListener() {
            @Override
            public void onPageScrolled(int position, float positionOffset, int positionOffsetPixels) {
                if( mPresenter != null ) {
                     mPresenter.onPageScrolled()
                } 
            }

            @Override
            public void onPageSelected(int position) {}

            @Override
            public void onPageScrollStateChanged(int state) {}
        });

Hope it helps

isamotiuc commented 6 years ago

@benoitletondor Thank you for you response 👍