Destroy TiActivity Presenter properly #35

passsy · 2016-11-03T00:06:30Z

I discovered that the Presenter of an Activity wasn't destroyed properly. When changing the configuration of an Activity without closing it (by calling recreate() or changing the orientation) the Presenter was accidentally destroyed although the flag isChangingConfigurations was true.

I wrote tests for all permutations (with/without savior | don't keep Activities true/false) for the usecases:

changing configuration,
move to background and back to foreground and
finish the Activity.

see TiActivityPresenterDestroyTest.java

To verify everything is working end to end I wrote a UI tests (for the plugin module). It's a new module because I don't want to add a TestActivity to the debug build type of the plugin module. The test was failing before the fix was included.

This PR does not include a fix for TiFragment as it was reported in #33. A separate PR will follow targeting this problem

After a big test session I found two bugs. Tests for all states shows the same bugs.

# Conflicts: # plugin/build.gradle # thirtyinch/src/main/java/net/grandcentrix/thirtyinch/TiFragment.java # thirtyinch/src/main/java/net/grandcentrix/thirtyinch/TiPresenter.java # thirtyinch/src/main/java/net/grandcentrix/thirtyinch/internal/TiActivityDelegate.java move test for module plugin into a separate module plugin-test

will be part of a separate PR

passsy · 2016-11-03T00:07:55Z

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

-                !config.useStaticSaviorToRetain()
+        if (!destroyPresenter
+                && !config.useStaticSaviorToRetain()
+                && !mTiActivity.isActivityChangingConfigurations()


The fix itself was just this && !mTiActivity.isActivityChangingConfigurations() line.

StefMa · 2016-11-04T07:23:25Z

plugin-test/README.md

@@ -0,0 +1,8 @@
+## Test module for `plugin` module
+
+This module contains the tests androidTests for the `plugin` module.


Wrap androidTests into codeblock: androidTests

StefMa · 2016-11-04T07:24:21Z

plugin-test/README.md

+This reduces problems for users of the library but makes testing harder.
+
+Also, Activities cannot be defined in the `androidTest` package.
+Moving them into the `debug` buildType doesn't feel right.


Remove this line. It feels not professional to say something like

"doesn't feel right"

StefMa · 2016-11-04T07:27:38Z

plugin-test/src/androidTest/java/net/grandcentrix/thirtyinch/plugin_test/TiPluginTest.java

+        final Instrumentation instrumentation = InstrumentationRegistry.getInstrumentation();
+
+        // start the activity for the first time
+        final Intent intent = new Intent(instrumentation.getTargetContext(), TestActivity.class);


Duplicated code. Move this into its own method and return the TestActivity

StefMa · 2016-11-04T07:28:40Z

plugin-test/src/main/java/net/grandcentrix/thirtyinch/plugin_test/TestActivity.java

+import android.widget.TextView;
+
+/**
+ * only for tests


Remove this. This should be clear.

StefMa · 2016-11-04T07:31:27Z

plugin-test/src/main/java/net/grandcentrix/thirtyinch/plugin_test/TestPresenter.java

+    private int wakeupCalls = 0;
+
+    @Override
+    protected void onWakeUp() {


Use onAttachView(View) 🤔

StefMa · 2016-11-04T07:44:58Z

sample/src/main/java/net/grandcentrix/thirtyinch/sample/HelloWorldPresenter.java

    private PublishSubject<Void> triggerHeavyCalculation = PublishSubject.create();

-    private RxTiPresenterSubscriptionHandler rxSubscriptionHelper = new RxTiPresenterSubscriptionHandler(this);
+    public HelloWorldPresenter() {


Just for demonstration, right?
Otherwise we can remove it...

StefMa · 2016-11-04T07:45:54Z

sample/src/main/java/net/grandcentrix/thirtyinch/sample/SamplePresenter.java

    private RxTiPresenterSubscriptionHandler mSubscriptionHandler
            = new RxTiPresenterSubscriptionHandler(this);

+    public SamplePresenter() {


Same like above, demo right?

StefMa · 2016-11-04T07:49:30Z

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

@@ -151,6 +151,18 @@ public View onCreateView(final LayoutInflater inflater, @Nullable final ViewGrou
    @Override
    public void onDestroy() {
        super.onDestroy();
+        //FIXME handle attach/detach state
+        TiLog.v(TAG, "isChangingConfigurations = " + getActivity().isChangingConfigurations());


Please move all the Log method into it's own method like logFragmentInformation or something like that...

StefMa · 2016-11-04T07:53:10Z

thirtyinch/src/test/java/net/grandcentrix/thirtyinch/internal/PresenterSaviorTestHelper.java

+public class PresenterSaviorTestHelper {
+
+    /**
+     * helper to clear the savior without exposing this to the public api


Remove this info but add javadoc to the top of the class.

StefMa · 2016-11-04T08:02:14Z

...yinch/src/test/java/net/grandcentrix/thirtyinch/internal/TiActivityPresenterDestroyTest.java

+        }
+    }
+
+    private class Delegate extends TiActivityDelegate<TestPresenter, TiView> {


It would be better if we move this into its own class (instead of inner class) and setup all the stuff in the constructor via a builder pattern.
These lines looks really crazy and no one knows which parameter is for what :D

…destroy_presenter_properly

StefMa · 2016-11-06T12:18:29Z

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

+
+        final TiConfiguration config = mPresenter.getConfig();
+
+        if (DEBUG) {


This is still valid :)
Don't put all the Logs into the method. Create a own method instead which will log all the stuff.

StefMa · 2016-11-06T12:20:11Z

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

+    /**
+     * enables debug logging during development
+     */
+    private static final boolean DEBUG = false;


DEBUG is a little bit confused. It sounds like "debug product flavor" or something like that.
It is more a ENABLE_LOGGING, isn't it? 🤔

# Conflicts: # thirtyinch/build.gradle

passsy · 2016-11-07T14:38:05Z

The UI tests are very unstable on travis. I think I should remove them 😒

passsy · 2016-11-07T16:33:29Z

Ok, I fixed it. Ran it 3x and it works without problems

Pascal Welsch added 10 commits October 5, 2016 17:38

Test plugin activity recreate

290583a

Work in progress

92924e8

Added tests to preserve the current (wrong) state

3510037

After a big test session I found two bugs. Tests for all states shows the same bugs.

Add finish tests

584fbd0

Fix leaking presenter in some cases

9cc8966

Cleanup

66696c9

Cleanup accidentally touched code

b11a0d9

Restore old "buggy" fragment destroy logic

d4cefc1

will be part of a separate PR

Rename test and make it Activity specific

b09143a

passsy added thirtyinch plugin labels Nov 3, 2016

passsy commented Nov 3, 2016

View reviewed changes

Simplify UI tests, only check the textview text

755e100

StefMa suggested changes Nov 4, 2016

View reviewed changes

passsy added this to the 0.8.0 milestone Nov 4, 2016

Pascal Welsch added 4 commits November 4, 2016 14:00

Update README.md

6faf7af

Cleanup code according to review

3ea3308

Replace cryptic test setup with builder

c93200f

Merge commit 'ef4ed6793edbdb27f3cec713ea22501491a4ce55' into feature/…

5c77ff3

…destroy_presenter_properly

StefMa suggested changes Nov 6, 2016

View reviewed changes

Pascal Welsch added 2 commits November 7, 2016 15:08

Merge branch 'master' into feature/destroy_presenter_properly

e4c9977

# Conflicts: # thirtyinch/build.gradle

Extract debug loggin in separate method

23e9fa5

Add screen unlocking testrunner for stable travis CI builds

b0ce381

passsy assigned StefMa Nov 7, 2016

StefMa approved these changes Nov 8, 2016

View reviewed changes

StefMa merged commit 7e033cb into master Nov 8, 2016

StefMa deleted the feature/destroy_presenter_properly branch November 8, 2016 09:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Destroy TiActivity Presenter properly #35

Destroy TiActivity Presenter properly #35

passsy commented Nov 3, 2016 •

edited

Loading

passsy Nov 3, 2016

StefMa Nov 4, 2016

StefMa Nov 4, 2016

StefMa Nov 4, 2016

StefMa Nov 4, 2016

StefMa Nov 4, 2016

StefMa Nov 4, 2016

StefMa Nov 4, 2016

StefMa Nov 4, 2016

StefMa Nov 4, 2016

StefMa Nov 4, 2016

StefMa Nov 6, 2016

StefMa Nov 6, 2016

passsy commented Nov 7, 2016

passsy commented Nov 7, 2016

		@@ -0,0 +1,8 @@
		## Test module for `plugin` module

		This module contains the tests androidTests for the `plugin` module.


		final TiConfiguration config = mPresenter.getConfig();

		if (DEBUG) {

Destroy TiActivity Presenter properly #35

Destroy TiActivity Presenter properly #35

Conversation

passsy commented Nov 3, 2016 • edited Loading

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 Nov 7, 2016

passsy commented Nov 7, 2016

passsy commented Nov 3, 2016 •

edited

Loading