Presenter not destroyed when Fragment is popped off the backstack about thirtyinch HOT 12 CLOSED

It's the last missing piece for the 0.8.0 release and will be targeted next. I can't give any time frame.

from thirtyinch.

I don't really understand why TiFragment#onDestroy() has all that logic to decide whether or not to destroy the Presenter.

If config.shouldRetainPresenter() is true, which is the default, then the fragment has setRetainInstance(true) set in its onCreate() anyway, so the Fragment won't get destroyed on a normal configuration change. If Fragment.onDestroy() is being called by the framework, it's because the Fragment really is being properly destroyed and will not be recreated, so we should destroy the presenter too.

from thirtyinch.

Good catch, will investigate it

from thirtyinch.

I can verify this problem exists. But the fix isn't as easy always calling Presenter.destroy() in TiFragment#onDestroy(). The following states must be tested:

• with/without savior
• with/without retain
• don't keep activities enabled/disabled
• fragment in backstack
• fragment attached/detached

for the following usecases:

• call finish()
• bring Activity in background and back to foreground
• orientation change

Overall 72 cases, which should be tested. It was easier for the Activity where only 8 states are possible for 3 usecases (24 test). One reason I don't use Fragments anymore 😈
This also requires a delegate for TiFragment, otherwise it's not testable.
//cc #20

from thirtyinch.

Interestingly, there is a bugfix in SupportLibrary 25.1.0

• Fragment.onDestroy() not called for fragment in backstack

Maybe this caused some of the troubles

from thirtyinch.

@passsy onDestroy() is reliably called each time a fragment gets popped off the back stack. Even in support library version 24.2.1 which is currently used by ThirtyInch.
Sadly the bugfix description does not provide any helpful informations (for example under what conditions onDestroy() was not called) :(. Nevertheless the memory leak which is described in this issue can be verified and needs to be fixed.

from thirtyinch.

^ s/@passy/@passsy/ :)

from thirtyinch.

Because I run into the same issue I want to share some stuff.

We have a simple MasterDetail-Activity.
On phones we display the MasterFragment. On click we replace it with the DetailFragment.
On back press we pop it (because of backstack stuff).

In tablets landscape we display the MasterFragment on the left side and when we click on an item we put the DetailFragment on the right side.
On tablets portrait we have the same behaviour like phones.
Which means we have to detect the orientation change on rotation and:
Land -> Port: Remove the DetailFragment form the DetailContainer and replace it to the MasterContainer
Port -> Land: Remove the DetailFragment from the MasterContainer and replace it to the DetailContainer.

On our DetailPresenter I've put the following logic to fix the destroying:

    @Override
    public void onDestroy() {
        // Destroy presenter on phones (because they got detached). Otherwise they don't will destroy on backstackpress.
        // On tablets we put on orientation change the fragment from one container to another.
        // So we don't have to destroy the presenter but when the activity got finished
        // see https://git.io/vMzfA
        if (Utils.isPhone(getContext())) {
            MLog.d(TAG, "Destroy on Phones");
            getPresenter().destroy();
        }
        if (!mChangingConfigurations && Utils.isTablet(getContext())) {
            getPresenter().destroy();
            MLog.d(TAG, "Destroy on Tablets only when no configuration changed!");
        }
        super.onDestroy();
    }

    @Override
    public void onStop() {
        super.onStop();
        mChangingConfigurations = ((TiActivity) getActivity()).isActivityChangingConfigurations();
    }

Everything is tested with AppCompat 25.1.0

from thirtyinch.

Do we have an estimate for when a fix might be available? If it's not within the next 2-3 weeks, I'll have to find an alternative solution. :(

from thirtyinch.

Just to bump this issue:
@gborland As you may have noticed we are working hard on fixing (#78) this issue. Please feel free to try it out and provide your feedback.

from thirtyinch.

#78 is finally merged into master.
Thank you @gborland for the report and the testing 👍

We will publish a new release soon.

from thirtyinch.

v0.8.0-rc4 published

from thirtyinch.