Bugfix: Presenter not destroyed when Fragment is popped off the backstack #78

ghost · 2017-03-20T09:06:30Z

Hello everyone,

currently I'm working on a fix for #33.
We thought it would be better if we add all relevant Unit-Tests first so it would be easier to develop the bugfix. Additionally I've created a new Activity for the "Sample" application which can be be used to interactively test the Fragment lifecycle under different circumstances (e.g. back stack or configuration change).

I would like to open this PR as early as possible to make it open for discussion.

Please feel free to comment and contribute!

…e TiFragmentDelegate class.

…ntation.

…stroyed

…test class for all methods that don’t require the PutInMapAnswer.

…null Bundle.

…Activity.

…false.

…ations() and isFinishing() in tests

…est name.

I didn’t made it final so somebody could work around this restriction

Added a buch of new tests for TiActivity covering all destroy cases Removed “Don’t keep Activitites check”. It can be detected by checking isFinishing and isChangingConfigurations. isFinishing is false when the activity moves to background even when “don’t keep activities” is enabled. Removed support for NonConfigurationInstance. The savior works much better.

passsy · 2017-04-24T00:43:31Z

I was able to simplify the onDestroy() logic in TiFragment and TiActivity. I also added all required tests for the TiActivity.

Also:

removed the NonConfigurationInstance recover method. setUseStaticSaviorToRetain(false) is deprecated and therefore it was never used. The savior does a better job.
removed hard "Don't keep Activities" checks. This is not required anymore. isFinishing and isChangingConfigurations are enough information. The "Don't keep Activities" check was unreachable code.
TiFragment#setRetainInstanceState(true) will now throw and is not supported.
added documentation

I'm now happy with the result and look forward for the merge. Next step: write release notes 😁

all fixed, outdated

ar-g

Looked through PR, not very familiar to leave more suggestions yet :)
The PR fixed an issue in our app.

ar-g · 2017-04-19T17:00:44Z

thirtyinch/src/main/java/net/grandcentrix/thirtyinch/internal/DelegatedTiFragment.java

     */
-    void setFragmentRetainInstance(final boolean retain);
+    boolean isInBackstack();


isFragmentInBackstack() may fit better

ar-g · 2017-04-24T08:36:22Z

thirtyinch/src/main/java/net/grandcentrix/thirtyinch/internal/ActivityScopedPresenters.java

+import java.util.List;
+import java.util.Map;
+
+public class ActivityScopedPresenters {


ActivityScopedPresenters not necessary but it makes code more readable

StefMa

I haven't yet looked into the updated code.
But I got always the following output, when I destroy an activity:

Stacktrace

04-25 09:23:54.032 23793-23793/net.grandcentrix.thirtyinch.sample V/ThirtyInch: SampleFragment:TiFragment@cd04102: binding the cached view to Presenter MainThreadProxy@d1f9626-SampleFragment@cd04102{presenter=SamplePresenter@2067a13}
04-25 09:23:54.033 23793-23793/net.grandcentrix.thirtyinch.sample V/ThirtyInch: SamplePresenter:TiPresenter@2067a13: onAttachView(TiView)
04-25 09:23:54.033 23793-23793/net.grandcentrix.thirtyinch.sample V/ThirtyInch: SamplePresenter:TiPresenter@2067a13: deprecated onWakeUp()
04-25 09:23:54.033 23793-23793/net.grandcentrix.thirtyinch.sample V/ThirtyInch: HelloWorldActivity:TiActivity@f765445: binding the cached view to Presenter DistinctUntilChangedProxy@edd875f-MainThreadProxy@520e03-HelloWorldActivity:TiActivity@f765445{presenter = HelloWorldPresenter@acefc1}
04-25 09:23:54.033 23793-23793/net.grandcentrix.thirtyinch.sample V/ThirtyInch: HelloWorldPresenter:TiPresenter@acefc1: onAttachView(TiView)
04-25 09:23:54.034 23793-23793/net.grandcentrix.thirtyinch.sample V/ThirtyInch: HelloWorldPresenter:TiPresenter@acefc1: deprecated onWakeUp()
04-25 09:23:54.409 23793-23793/net.grandcentrix.thirtyinch.sample V/ThirtyInch: SamplePresenter:TiPresenter@e9e1afa: deprecated onSleep()
04-25 09:23:54.409 23793-23793/net.grandcentrix.thirtyinch.sample V/ThirtyInch: SamplePresenter:TiPresenter@e9e1afa: onDetachView()
04-25 09:23:54.409 23793-23793/net.grandcentrix.thirtyinch.sample V/ThirtyInch: HelloWorldPresenter:TiPresenter@7132889: deprecated onSleep()
04-25 09:23:54.409 23793-23793/net.grandcentrix.thirtyinch.sample V/ThirtyInch: HelloWorldPresenter:TiPresenter@7132889: onDetachView()
04-25 09:23:54.409 23793-23793/net.grandcentrix.thirtyinch.sample V/ThirtyInch: ActivityInstanceObserver: destroying HelloWorldActivity:TiActivity@7d2b253{presenter = HelloWorldPresenter@7132889}
04-25 09:23:54.409 23793-23793/net.grandcentrix.thirtyinch.sample V/ThirtyInch: ActivityInstanceObserver: isFinishing = true
04-25 09:23:54.409 23793-23793/net.grandcentrix.thirtyinch.sample V/ThirtyInch: ActivityInstanceObserver: isChangingConfigurations = false
04-25 09:23:54.409 23793-23793/net.grandcentrix.thirtyinch.sample D/ThirtyInch: PresenterSavior: Activity is finishing, free remaining presenters HelloWorldActivity:TiActivity@7d2b253{presenter = HelloWorldPresenter@7132889}
04-25 09:23:54.410 23793-23793/net.grandcentrix.thirtyinch.sample V/ThirtyInch: SamplePresenter:TiPresenter@e9e1afa: onDestroy()
04-25 09:23:54.410 23793-23793/net.grandcentrix.thirtyinch.sample D/ThirtyInch: ActivityScopedPresenters@c78308e: remove SamplePresenter:245242618:1798809768000 SamplePresenter:TiPresenter@e9e1afa{view = null}
04-25 09:23:54.410 23793-23793/net.grandcentrix.thirtyinch.sample V/ThirtyInch: HelloWorldPresenter:TiPresenter@7132889: onDestroy()
04-25 09:23:54.410 23793-23793/net.grandcentrix.thirtyinch.sample D/ThirtyInch: ActivityScopedPresenters@c78308e: remove HelloWorldPresenter:118696073:1798798494000 HelloWorldPresenter:TiPresenter@7132889{view = null}
04-25 09:23:54.410 23793-23793/net.grandcentrix.thirtyinch.sample V/ThirtyInch: SamplePresenter:TiPresenter@e9e1afa: not calling onDetachView(), not woken up
04-25 09:23:54.411 23793-23793/net.grandcentrix.thirtyinch.sample V/ThirtyInch: SampleFragment:TiFragment@e45c925: Hosting Activity is finishing, destroying presenter SamplePresenter:TiPresenter@e9e1afa{view = null}
04-25 09:23:54.411 23793-23793/net.grandcentrix.thirtyinch.sample W/ThirtyInch: SamplePresenter:TiPresenter@e9e1afa: not calling onDestroy(), destroy was already called
04-25 09:23:54.412 23793-23793/net.grandcentrix.thirtyinch.sample V/ThirtyInch: HelloWorldActivity:TiActivity@7d2b253: Activity is finishing, destroying presenter HelloWorldPresenter:TiPresenter@7132889{view = null}
04-25 09:23:54.412 23793-23793/net.grandcentrix.thirtyinch.sample W/ThirtyInch: HelloWorldPresenter:TiPresenter@7132889: not calling onDestroy(), destroy was already called

The

HelloWorldPresenter:TiPresenter@7132889: not calling onDestroy(), destroy was already called

makes me a little bit stomach pain...

It seems that we calling destroy() multiple times 🤔
That needs to be fixed...

How to test:
Add to the sample:
startActivity(new Intent(HelloWorldActivity.this, HelloWorldActivity.class));
In recreate click listener and remove recreate().
Then start the sample. Click on recreate button and press back.

Update:

This behaviour is "ok".
We have discussed that internally.
See also #78 (comment)

StefMa · 2017-04-25T07:40:23Z

...ain/java/net/grandcentrix/thirtyinch/sample/fragmentlifecycle/FragmentLifecycleActivity.java

+    private boolean isDontKeepActivities() {
+        // default behaviour
+        int dontKeepActivities = 0;
+        try {


You can use your util here now. Otherwise AndroidDeveloperOptions is not used and can removed

StefMa · 2017-04-25T07:43:43Z

sample/src/main/java/net/grandcentrix/thirtyinch/sample/util/AndroidDeveloperOptions.java

+                            0);
+        }
+
+        return alwaysFinishActivitiesInt == 1;


If we use that in our sample:
Change that to alwaysFinishActivitiesInt != DEFAULT_VALUE.

StefMa · 2017-04-25T07:44:06Z

sample/src/main/java/net/grandcentrix/thirtyinch/sample/util/AndroidDeveloperOptions.java

+        if (Build.VERSION.SDK_INT >= 17) {
+            alwaysFinishActivitiesInt = Settings.System
+                    .getInt(context.getContentResolver(), Settings.Global.ALWAYS_FINISH_ACTIVITIES,
+                            0);


If we use that.
Move this zero and the zero below to an constant like DEFAULT_VALUE = -1

StefMa · 2017-04-25T07:52:30Z

thirtyinch/src/main/java/net/grandcentrix/thirtyinch/TiFragment.java

+ * {@link FragmentManager}. When {@link FragmentTransaction#remove(Fragment)} or {@link
+ * FragmentTransaction#replace(int, Fragment)} results in removing this {@link TiFragment} from the
+ * {@link FragmentManager} the {@link TiPresenter} gets destroyed. When the same {@link TiFragment}
+ * instance will be added again a new {@link TiPresenter} will be created by calling {@link


That is not true.
Replace the following in our sample app:

- recreate() + final Fragment fragment = getSupportFragmentManager() + .findFragmentById(R.id.fragment_container); + getSupportFragmentManager().beginTransaction().remove(fragment).commitNow(); + Observable.just(null).delay(3, TimeUnit.SECONDS).subscribe( + o -> getSupportFragmentManager().beginTransaction() + .add(R.id.fragment_container, fragment).addToBackStack(null) + .commit());

I got the following output:

Logcat

04-25 09:50:41.876 5895-5895/net.grandcentrix.thirtyinch.sample V/ThirtyInch: SamplePresenter:TiPresenter@70beced: deprecated onSleep() 04-25 09:50:41.876 5895-5895/net.grandcentrix.thirtyinch.sample V/ThirtyInch: SamplePresenter:TiPresenter@70beced: onDetachView() 04-25 09:50:41.876 5895-5895/net.grandcentrix.thirtyinch.sample V/ThirtyInch: SamplePresenter:TiPresenter@70beced: not calling onDetachView(), not woken up 04-25 09:50:44.881 5895-5895/net.grandcentrix.thirtyinch.sample V/ThirtyInch: CallOnMainThreadInterceptor: wrapping View SampleFragment@cd04102{presenter=SamplePresenter@70beced} in MainThreadProxy@60c384c-SampleFragment@cd04102{presenter=SamplePresenter@70beced} 04-25 09:50:44.881 5895-5895/net.grandcentrix.thirtyinch.sample V/ThirtyInch: DistinctUntilChangedInterceptor: wrapping View MainThreadProxy@60c384c-SampleFragment@cd04102{presenter=SamplePresenter@70beced} in MainThreadProxy@60c384c-SampleFragment@cd04102{presenter=SamplePresenter@70beced} 04-25 09:50:44.881 5895-5895/net.grandcentrix.thirtyinch.sample V/ThirtyInch: CallOnMainThreadInterceptor: wrapping View MainThreadProxy@60c384c-SampleFragment@cd04102{presenter=SamplePresenter@70beced} in MainThreadProxy@3235095-MainThreadProxy@60c384c-SampleFragment@cd04102{presenter=SamplePresenter@70beced} 04-25 09:50:44.881 5895-5895/net.grandcentrix.thirtyinch.sample V/ThirtyInch: DistinctUntilChangedInterceptor: wrapping View MainThreadProxy@3235095-MainThreadProxy@60c384c-SampleFragment@cd04102{presenter=SamplePresenter@70beced} in MainThreadProxy@3235095-MainThreadProxy@60c384c-SampleFragment@cd04102{presenter=SamplePresenter@70beced} 04-25 09:50:44.881 5895-5895/net.grandcentrix.thirtyinch.sample V/ThirtyInch: SampleFragment:TiFragment@cd04102: binding NEW view to Presenter MainThreadProxy@3235095-MainThreadProxy@60c384c-SampleFragment@cd04102{presenter=SamplePresenter@70beced} 04-25 09:50:44.881 5895-5895/net.grandcentrix.thirtyinch.sample V/ThirtyInch: SamplePresenter:TiPresenter@70beced: onAttachView(TiView) 04-25 09:50:44.881 5895-5895/net.grandcentrix.thirtyinch.sample V/ThirtyInch: SamplePresenter:TiPresenter@70beced: deprecated onWakeUp()

You see the same (TiPresenter@70beced) presenter instance will be used

I added this case to the fragment lifecycle test Activity and can't reproduce it. The presenter is always destroyed and a new one is generated.

That was discussed and had the right behaviour.
The second (or following) added fragment got the same presenter.
Why? Because we added it to the backstack. And the default behaviour of TiFragments-Presenter is to keep as long as you are managed by the fragment manager. And if you are in the backstack, you are managed by them ...

StefMa · 2017-04-25T07:54:30Z

thirtyinch/src/main/java/net/grandcentrix/thirtyinch/TiFragment.java

+ * {@link TiPresenter} will be destroyed accordingly.
+ * </p>
+ * <p>
+ * Using {@code setRetainInstance(true)} is now allowed as it causes many troubles. You should favor


is ~~now~~ not allowed

StefMa · 2017-04-25T07:56:07Z

thirtyinch/src/main/java/net/grandcentrix/thirtyinch/internal/ActivityInstanceObserver.java

+    @Override
+    public void onActivityDestroyed(final Activity activity) {
+
+        // TODO check if "Don't keep Activities" case is handled correctly


Is that todo done?

StefMa · 2017-04-25T08:04:19Z

thirtyinch/src/main/java/net/grandcentrix/thirtyinch/internal/ActivityScopedPresenters.java

+/**
+ * Simple wrapper around a {@link HashMap} to save {@link TiPresenter} by id. For every {@link
+ * android.app.Activity} containing a {@link TiPresenter} a corresponding {@link
+ * ActivityScopedPresenters} will be created


Please add the info, that this will store also the Presenters for the Fragments which are added to that Activity.

StefMa · 2017-04-25T08:23:59Z

thirtyinch/src/main/java/net/grandcentrix/thirtyinch/internal/ActivityScopedPresenters.java

+ * android.app.Activity} containing a {@link TiPresenter} a corresponding {@link
+ * ActivityScopedPresenters} will be created
+ */
+public class ActivityScopedPresenters {


I think about implementing Map directly or extending from HashMap. But not needed now. So keep it like it is ;)

StefMa · 2017-04-25T08:30:22Z

thirtyinch/src/main/java/net/grandcentrix/thirtyinch/internal/PresenterSavior.java

+    /**
+     * Access to the {@link PresenterSavior} singleton to save presenters across orientation changes
+     *
+     * @return singleton for


remove that. I guess that is clear :D

StefMa · 2017-04-25T08:34:34Z

thirtyinch/src/main/java/net/grandcentrix/thirtyinch/internal/PresenterSavior.java

-    public void free(final String presenterId) {
-        mPresenters.remove(presenterId);
+    public void onActivityFinished(final Activity activity, final String activityId) {
+        final ActivityScopedPresenters scope = mScopes.remove(activityId);


Move that below the statement mScope.isEmpty(). We need it only after this statement...

the scope is only required in on branch, true. But removing the scope is the important part here. required for both branches.

Then maybe moving up the second statement? 🤔

passsy · 2017-04-25T11:05:26Z

multiple calls to TiPresenter#destroy() are ok. We have multiple triggers to detect the Activity destroy case and we can't say which one fires first. We could guard them with if (!presenter.isDestroyed()) { presenter.destroy(); }. I don't think this is useful.

This happens every time because of multiple triggers (Savior, Fragment and Activity)

StefMa · 2017-04-25T12:28:14Z

I've checked the ViewPager behaviour like we discussed.
See #84

…r_not_destroyed Add sample/test for viewpager

Robert Berghegger and others added 30 commits March 10, 2017 14:38

Adds a TiFragmentDelegateBuilder.

f0b5a3d

Changes the visibility of the SAVED_PRESENTER_ID class variable in th…

d482f01

…e TiFragmentDelegate class.

Adds a TiFragmentPresenterDestroyTest class with a first test impleme…

b6e0f67

…ntation.

Merge branch 'master' into bugfix/fragment_backstack_presenter_not_de…

34b803e

…stroyed

Inject savior with fragment delegate builder

c41603c

Adds more tests to the TiFragmentPresenterDestroyTest class.

81323af

Refactors method names in the TiFragmentDelegateBuilder class.

0f87776

Changes the back stack test case.

b1c9b70

Adds a fragment lifecycle test to the sample application.

788f4c4

Logs delegate method callbacks in the TestFragment.

edf1db7

Changes tests according to the real Fragment lifecycle.

a7cac19

Adds more tests to the TiFragmentPresenterDestroyTest.

92b3c68

Change name of sample app

89c7a25

Add all test cases stubs

c957464

Moves the basic Bundle initialization into the setUp() method of the …

ccbb6c6

…test class for all methods that don’t require the PutInMapAnswer.

Logs if onCreate_afterSuper should be called with null or with a non …

831fc34

…null Bundle.

Adds an option to retain a fragment instance in the FragmentLifecycle…

0758938

…Activity.

Improves logging in the TestFragment.

c9557c0

Implements all tests for the combination savior = false and retain = …

04179dc

…false.

Fixes false test implementations.

4ff89db

Add HostingActivity which can change it’s state of isChangingConfigur…

803b7b3

…ations() and isFinishing() in tests

Fixes test cases so they only test the scope that is implied by the t…

1f32468

…est name.

Adds more test case implementations.

1a8b791

Adds missing test implementations.

c5f831d

Improve logging

2497ca7

Splits the tests into multiple files.

5dda802

Adds more missing test cases.

fd16f88

Adds more missing test cases.

05f2541

Implement equals correctly for the configuration

838ab48

Add more logging

ea700d8

Pascal Welsch added 7 commits April 23, 2017 23:31

Test to reuse of fragments

a8b6c61

Remove todo which is now done

52e02c6

Cleanup logging

96c5f93

Remove all deprecated usages of setUseStaticSaviorToRetain()

f49152a

Add documentation

e46621e

Don’t allow the usage of setRetainInstance(true)

3fac777

I didn’t made it final so somebody could work around this restriction

ar-g reviewed Apr 24, 2017

View reviewed changes

Pascal Welsch added 2 commits April 24, 2017 11:47

Rename isInBackstack -> isFragmentInBackstack

c3997c8

Improve providePresenter documentation

191bb5d

StefMa added the Ready for Review label Apr 25, 2017

StefMa suggested changes Apr 25, 2017

View reviewed changes

Pascal Welsch added 3 commits April 25, 2017 12:01

Remove unused class (was moved to sample)

6d67cbf

Review improvements

ab8f873

Add detach/attach feature to sample

9cd36cb

Pascal Welsch and others added 3 commits April 25, 2017 13:57

Don’t print a hint as warning

43aa76a

This happens every time because of multiple triggers (Savior, Fragment and Activity)

Use same visibility for Listener as the using class

a89f8de

Add sample/test for viewpager

cf4d1d4

StefMa mentioned this pull request Apr 25, 2017

Add sample/test for viewpager #84

Merged

StefMa and others added 2 commits April 25, 2017 14:35

Better starting of Activities

143a378

Merge pull request #84 from StefMa/bugfix/fragment_backstack_presente…

3880e06

…r_not_destroyed Add sample/test for viewpager

StefMa approved these changes Apr 25, 2017

View reviewed changes

StefMa merged commit 57fc2af into master Apr 25, 2017

passsy deleted the bugfix/fragment_backstack_presenter_not_destroyed branch April 25, 2017 16:28

passsy mentioned this pull request Apr 26, 2017

Feature/presenter host refactoring #88

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix: Presenter not destroyed when Fragment is popped off the backstack #78

Bugfix: Presenter not destroyed when Fragment is popped off the backstack #78

ghost commented Mar 20, 2017 •

edited by passsy

Loading

passsy commented Apr 24, 2017

ar-g left a comment

ar-g Apr 19, 2017

ar-g Apr 24, 2017

StefMa left a comment •

edited

Loading

StefMa Apr 25, 2017

StefMa Apr 25, 2017

StefMa Apr 25, 2017

StefMa Apr 25, 2017

passsy Apr 25, 2017

StefMa Apr 25, 2017

StefMa Apr 25, 2017

StefMa Apr 25, 2017

StefMa Apr 25, 2017

StefMa Apr 25, 2017

StefMa Apr 25, 2017

StefMa Apr 25, 2017

passsy Apr 25, 2017

StefMa Apr 25, 2017

passsy commented Apr 25, 2017

StefMa commented Apr 25, 2017

Bugfix: Presenter not destroyed when Fragment is popped off the backstack #78

Bugfix: Presenter not destroyed when Fragment is popped off the backstack #78

Conversation

ghost commented Mar 20, 2017 • edited by passsy Loading

passsy commented Apr 24, 2017

ar-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StefMa left a comment • edited Loading

Choose a reason for hiding this comment

Update:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

passsy commented Apr 25, 2017

StefMa commented Apr 25, 2017

ghost commented Mar 20, 2017 •

edited by passsy

Loading

StefMa left a comment •

edited

Loading