From c157d19c9a2229094530acb2006124d8544d4137 Mon Sep 17 00:00:00 2001 From: Kejun Xia Date: Sun, 29 Nov 2015 23:47:03 +1100 Subject: [PATCH] Investigating back nav injection issue --- .../lib/android/mvc/__MvcGraphHelper.java | 47 +++++------ .../mvc/controller/NavigationController.java | 10 ++- .../internal/NavigationControllerImpl.java | 9 ++- .../lib/android/mvc/TestMvcGraphHelper.java | 13 +-- .../mvc/view/nav/TestCaseNavigationBasic.java | 17 +++- .../nav/TestCaseNavigationFromController.java | 80 +++++++++++++------ .../lib/android/mvc/view/MvcActivity.java | 33 +++++++- .../lib/android/mvc/view/MvcFragment.java | 5 +- 8 files changed, 154 insertions(+), 60 deletions(-) diff --git a/library/android-mvc-controller/src/main/java/com/shipdream/lib/android/mvc/__MvcGraphHelper.java b/library/android-mvc-controller/src/main/java/com/shipdream/lib/android/mvc/__MvcGraphHelper.java index e859b4f..79f1e7c 100644 --- a/library/android-mvc-controller/src/main/java/com/shipdream/lib/android/mvc/__MvcGraphHelper.java +++ b/library/android-mvc-controller/src/main/java/com/shipdream/lib/android/mvc/__MvcGraphHelper.java @@ -14,45 +14,46 @@ * Helper class to work with MvcGraph. Internal use only. Don't use it in your app. */ public class __MvcGraphHelper { - private static Map> retainedProviders = new HashMap<>(); + private static Map> retainedProviders = new HashMap<>(); /** * Internal use. Don't call it in app directly. */ public static void retainCachedObjectsBeforeNavigation( - NavigationController.EventC2V.OnLocationForward navigationEvent, MvcGraph mvcGraph) { - //Retain all cached items before navigation. - Collection cachedItems = mvcGraph.singletonScopeCache.getCachedItems(); + final NavigationController.EventC2V.OnLocationChanged navigationEvent, + final MvcGraph mvcGraph) { + //Retain all cached items before navigation. + Collection cachedItems = mvcGraph.singletonScopeCache.getCachedItems(); - List providers = new ArrayList<>(); - retainedProviders.put(navigationEvent, providers); - for (ScopeCache.CachedItem cachedItem : cachedItems) { - Provider provider = cachedItem.getProvider(); - if (provider != null) { - providers.add(provider); - provider.retain(); + List providers = new ArrayList<>(); + retainedProviders.put(navigationEvent, providers); + for (ScopeCache.CachedItem cachedItem : cachedItems) { + Provider provider = cachedItem.getProvider(); + if (provider != null) { + providers.add(provider); + provider.retain(); + } } - } } /** * Internal use. Don't call it in app directly. */ public static void releaseCachedItemsAfterNavigation( - NavigationController.EventC2V.OnLocationForward navigationEvent, MvcGraph mvcGraph) { - - //Release all cached items after the fragment navigated to is ready to show. - Collection providers = retainedProviders.get(navigationEvent); - if (providers != null) { - for (Provider provider : providers) { - if (provider != null) { - provider.release(); + final NavigationController.EventC2V.OnLocationChanged navigationEvent, + final MvcGraph mvcGraph) { + //Release all cached items after the fragment navigated to is ready to show. + Collection providers = retainedProviders.get(navigationEvent); + if (providers != null) { + for (Provider provider : providers) { + if (provider != null) { + provider.release(); + } } + providers.clear(); } - providers.clear(); - } - retainedProviders.remove(navigationEvent); + retainedProviders.remove(navigationEvent); } /** diff --git a/library/android-mvc-controller/src/main/java/com/shipdream/lib/android/mvc/controller/NavigationController.java b/library/android-mvc-controller/src/main/java/com/shipdream/lib/android/mvc/controller/NavigationController.java index 6caa270..c955073 100644 --- a/library/android-mvc-controller/src/main/java/com/shipdream/lib/android/mvc/controller/NavigationController.java +++ b/library/android-mvc-controller/src/main/java/com/shipdream/lib/android/mvc/controller/NavigationController.java @@ -104,10 +104,16 @@ public interface NavigationController extends BaseController { + public OnLocationChanged(Object sender, NavLocation lastValue, NavLocation currentValue) { + super(sender, lastValue, currentValue); + } + } + /** * Event to notify views navigation will move forward. */ - class OnLocationForward extends ValueChangeEventC2V { + class OnLocationForward extends OnLocationChanged { private boolean clearHistory; private NavLocation locationWhereHistoryClearedUpTo; @@ -147,7 +153,7 @@ public NavLocation getLocationWhereHistoryClearedUpTo() { /** * Event to notify views navigation will move backward. */ - class OnLocationBack extends ValueChangeEventC2V { + class OnLocationBack extends OnLocationChanged { private boolean fastRewind; public OnLocationBack(Object sender, NavLocation lastValue, NavLocation currentValue, diff --git a/library/android-mvc-controller/src/main/java/com/shipdream/lib/android/mvc/controller/internal/NavigationControllerImpl.java b/library/android-mvc-controller/src/main/java/com/shipdream/lib/android/mvc/controller/internal/NavigationControllerImpl.java index e335d94..39fb1b5 100644 --- a/library/android-mvc-controller/src/main/java/com/shipdream/lib/android/mvc/controller/internal/NavigationControllerImpl.java +++ b/library/android-mvc-controller/src/main/java/com/shipdream/lib/android/mvc/controller/internal/NavigationControllerImpl.java @@ -131,7 +131,14 @@ public void navigateBack(Object sender) { NavLocation previousLoc = currentLoc.getPreviousLocation(); getModel().setCurrentLocation(previousLoc); - postC2VEvent(new EventC2V.OnLocationBack(sender, currentLoc, previousLoc, false)); + + EventC2V.OnLocationBack navEvent = new EventC2V.OnLocationBack(sender, currentLoc, previousLoc, false); + + if (previousLoc != null ) { + __MvcGraphHelper.retainCachedObjectsBeforeNavigation(navEvent, Injector.getGraph()); + } + + postC2VEvent(navEvent); logger.trace("Nav Controller: Backward: {} -> {}", currentLoc.getLocationId(), previousLoc == null ? "null" : previousLoc.getLocationId()); diff --git a/library/android-mvc-controller/src/test/java/com/shipdream/lib/android/mvc/TestMvcGraphHelper.java b/library/android-mvc-controller/src/test/java/com/shipdream/lib/android/mvc/TestMvcGraphHelper.java index 2cda4e6..8e7e26f 100644 --- a/library/android-mvc-controller/src/test/java/com/shipdream/lib/android/mvc/TestMvcGraphHelper.java +++ b/library/android-mvc-controller/src/test/java/com/shipdream/lib/android/mvc/TestMvcGraphHelper.java @@ -150,6 +150,7 @@ protected ExecutorService createExecutorService() { ControllerA controllerAInViewA = viewA.controllerA; ManageA manageAInViewA = ((ControllerAImpl)viewA.controllerA).manageA; + //Simulate navigate to viewB NavigationController.EventC2V.OnLocationForward navEventB = mock( NavigationController.EventC2V.OnLocationForward.class); __MvcGraphHelper.retainCachedObjectsBeforeNavigation(navEventB, Injector.getGraph()); @@ -158,25 +159,27 @@ protected ExecutorService createExecutorService() { ControllerA controllerAInViewB = viewB.controllerA; ManageA manageAInViewB = ((ControllerAImpl)viewA.controllerA).manageA; + //release viewA Injector.getGraph().release(viewA); __MvcGraphHelper.releaseCachedItemsAfterNavigation(navEventB, Injector.getGraph()); verify(manageAMock, times(0)).onDisposed(); + //Simulate navigate to viewC NavigationController.EventC2V.OnLocationForward navEventC = mock( NavigationController.EventC2V.OnLocationForward.class); __MvcGraphHelper.retainCachedObjectsBeforeNavigation(navEventC, Injector.getGraph()); - Injector.getGraph().release(viewB); - __MvcGraphHelper.releaseCachedItemsAfterNavigation(navEventC, Injector.getGraph()); - verify(manageAMock, times(0)).onDisposed(); - Injector.getGraph().inject(viewC); ControllerA controllerAInViewC = viewB.controllerA; ManageA manageAInViewC = ((ControllerAImpl)viewA.controllerA).manageA; + //release viewB + Injector.getGraph().release(viewB); + __MvcGraphHelper.releaseCachedItemsAfterNavigation(navEventC, Injector.getGraph()); + verify(manageAMock, times(0)).onDisposed(); + Assert.assertTrue(manageAInViewA == manageAInViewB); Assert.assertTrue(manageAInViewA == manageAInViewC); - } } diff --git a/library/android-mvc-test/src/androidTest/java/com/shipdream/lib/android/mvc/view/nav/TestCaseNavigationBasic.java b/library/android-mvc-test/src/androidTest/java/com/shipdream/lib/android/mvc/view/nav/TestCaseNavigationBasic.java index 933d9da..d5e2620 100644 --- a/library/android-mvc-test/src/androidTest/java/com/shipdream/lib/android/mvc/view/nav/TestCaseNavigationBasic.java +++ b/library/android-mvc-test/src/androidTest/java/com/shipdream/lib/android/mvc/view/nav/TestCaseNavigationBasic.java @@ -28,6 +28,10 @@ import org.junit.Assert; import org.junit.Test; +import org.mockito.invocation.InvocationOnMock; +import org.mockito.stubbing.Answer; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import javax.inject.Inject; import javax.inject.Singleton; @@ -36,12 +40,16 @@ import static android.support.test.espresso.assertion.ViewAssertions.matches; import static android.support.test.espresso.matcher.ViewMatchers.isDisplayed; import static android.support.test.espresso.matcher.ViewMatchers.withText; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.reset; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; public class TestCaseNavigationBasic extends BaseTestCase { + private Logger logger = LoggerFactory.getLogger(getClass()); + @Inject private AnotherController anotherController; @@ -97,6 +105,13 @@ protected void injectDependencies(ScopeCache mvcSingletonCache) { super.injectDependencies(mvcSingletonCache); disposeCheckerAMock = mock(DisposeCheckerA.class); + doAnswer(new Answer() { + @Override + public Object answer(InvocationOnMock invocation) throws Throwable { + logger.debug("Dispose checker A"); + return null; + } + }).when(disposeCheckerAMock).onDisposed(); disposeCheckerBMock = mock(DisposeCheckerB.class); disposeCheckerCMock = mock(DisposeCheckerC.class); disposeCheckerDMock = mock(DisposeCheckerD.class); @@ -141,7 +156,7 @@ public void testShouldReleaseInjectionsAfterFragmentsArePoppedOut() throws Throw verify(disposeCheckerDMock, times(0)).onDisposed(); navigationController.navigateBack(this); waitTest(); - waitTest(1000); + waitTest(2000); verify(disposeCheckerAMock, times(1)).onDisposed(); verify(disposeCheckerBMock, times(1)).onDisposed(); verify(disposeCheckerCMock, times(1)).onDisposed(); diff --git a/library/android-mvc-test/src/androidTest/java/com/shipdream/lib/android/mvc/view/nav/TestCaseNavigationFromController.java b/library/android-mvc-test/src/androidTest/java/com/shipdream/lib/android/mvc/view/nav/TestCaseNavigationFromController.java index c1e16ea..42ebeed 100644 --- a/library/android-mvc-test/src/androidTest/java/com/shipdream/lib/android/mvc/view/nav/TestCaseNavigationFromController.java +++ b/library/android-mvc-test/src/androidTest/java/com/shipdream/lib/android/mvc/view/nav/TestCaseNavigationFromController.java @@ -22,25 +22,33 @@ import com.shipdream.lib.android.mvc.view.BaseTestCase; import com.shipdream.lib.poke.Component; import com.shipdream.lib.poke.Consumer; +import com.shipdream.lib.poke.Provides; import com.shipdream.lib.poke.ScopeCache; import org.junit.Test; +import org.mockito.invocation.InvocationOnMock; +import org.mockito.stubbing.Answer; +import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.util.Random; import javax.inject.Inject; +import javax.inject.Singleton; import static android.support.test.espresso.Espresso.onView; import static android.support.test.espresso.assertion.ViewAssertions.matches; import static android.support.test.espresso.matcher.ViewMatchers.isDisplayed; import static android.support.test.espresso.matcher.ViewMatchers.withText; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.reset; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; public class TestCaseNavigationFromController extends BaseTestCase { + private Logger logger = LoggerFactory.getLogger(getClass()); + @Inject private NavigationController navigationController; @@ -65,23 +73,23 @@ public static class Comp extends Component{ super(scopeCache); } -// @Singleton -// @Provides -// public DisposeCheckerE providesDisposeCheckerE() { -// return testCaseNavigation.disposeCheckerEMock; -// } -// -// @Singleton -// @Provides -// public DisposeCheckerF providesDisposeCheckerF() { -// return testCaseNavigation.disposeCheckerFMock; -// } -// -// @Singleton -// @Provides -// public DisposeCheckerG providesDisposeCheckerG() { -// return testCaseNavigation.disposeCheckerGMock; -// } + @Singleton + @Provides + public DisposeCheckerE providesDisposeCheckerE() { + return testCaseNavigation.disposeCheckerEMock; + } + + @Singleton + @Provides + public DisposeCheckerF providesDisposeCheckerF() { + return testCaseNavigation.disposeCheckerFMock; + } + + @Singleton + @Provides + public DisposeCheckerG providesDisposeCheckerG() { + return testCaseNavigation.disposeCheckerGMock; + } } @Override @@ -89,8 +97,29 @@ protected void injectDependencies(ScopeCache mvcSingletonCache) { super.injectDependencies(mvcSingletonCache); disposeCheckerEMock = mock(DisposeCheckerE.class); + doAnswer(new Answer() { + @Override + public Object answer(InvocationOnMock invocation) throws Throwable { + logger.debug("Dispose checker E"); + return null; + } + }).when(disposeCheckerEMock).onDisposed(); disposeCheckerFMock = mock(DisposeCheckerF.class); + doAnswer(new Answer() { + @Override + public Object answer(InvocationOnMock invocation) throws Throwable { + logger.debug("Dispose checker F"); + return null; + } + }).when(disposeCheckerFMock).onDisposed(); disposeCheckerGMock = mock(DisposeCheckerG.class); + doAnswer(new Answer() { + @Override + public Object answer(InvocationOnMock invocation) throws Throwable { + logger.debug("Dispose checker G"); + return null; + } + }).when(disposeCheckerGMock).onDisposed(); comp = new Comp(mvcSingletonCache); comp.testCaseNavigation = this; AndroidMvc.graph().register(comp); @@ -187,18 +216,19 @@ public void consume(ControllerG instance) { navigationController.navigateBack(this); onView(withText(valE)).check(matches(isDisplayed())); - waitTest(1000); + waitTest(); verify(disposeCheckerEMock, times(0)).onDisposed(); -// verify(disposeCheckerFMock, times(1)).onDisposed(); -// verify(disposeCheckerGMock, times(1)).onDisposed(); -// resetDisposeCheckers(); + LoggerFactory.getLogger(getClass()).debug("DisposeCheck, checking F"); + verify(disposeCheckerFMock, times(1)).onDisposed(); + //__MvcGraphHelper retaining all cache is dangerous. Try to only retain relevant injected instances. + verify(disposeCheckerGMock, times(0)).onDisposed(); + resetDisposeCheckers(); LoggerFactory.getLogger(getClass()).debug("DisposeCheck nav back"); navigationController.navigateBack(this); - waitTest(1000); -// verify(disposeCheckerEMock, times(1)).onDisposed(); -// verify(disposeCheckerFMock, times(0)).onDisposed(); -// verify(disposeCheckerGMock, times(0)).onDisposed(); + verify(disposeCheckerEMock, times(1)).onDisposed(); + verify(disposeCheckerFMock, times(0)).onDisposed(); + verify(disposeCheckerGMock, times(1)).onDisposed(); } private void resetDisposeCheckers() { diff --git a/library/android-mvc/src/main/java/com/shipdream/lib/android/mvc/view/MvcActivity.java b/library/android-mvc/src/main/java/com/shipdream/lib/android/mvc/view/MvcActivity.java index e752a5f..8bf4c5f 100644 --- a/library/android-mvc/src/main/java/com/shipdream/lib/android/mvc/view/MvcActivity.java +++ b/library/android-mvc/src/main/java/com/shipdream/lib/android/mvc/view/MvcActivity.java @@ -485,7 +485,7 @@ public void run() { } } - private void performBackNav(NavigationController.EventC2V.OnLocationBack event) { + private void performBackNav(final NavigationController.EventC2V.OnLocationBack event) { NavLocation currentLoc = event.getCurrentValue(); if (currentLoc == null) { MvcActivity mvcActivity = ((MvcActivity) getActivity()); @@ -510,6 +510,33 @@ private void performBackNav(NavigationController.EventC2V.OnLocationBack event) } } } + + MvcFragment lastFrag = null; + if (event.getLastValue() != null && event.getLastValue().getLocationId() != null) { + lastFrag = (MvcFragment) fm.findFragmentByTag( + getFragmentTag(event.getLastValue().getLocationId())); + } + final MvcFragment finalLastFrag = lastFrag; + + if (finalLastFrag != null) { + finalLastFrag.selfRelease = false; + } + + currentFrag.registerOnViewReadyListener(new Runnable() { + @Override + public void run() { + //Release reference count to pair the retaining by NavigationControllerImpl + // with Injector.getGraph().retainCachedObjectsBeforeNavigation(); + __MvcGraphHelper.releaseCachedItemsAfterNavigation(event, Injector.getGraph()); + + if (finalLastFrag != null) { + finalLastFrag.releaseDependencies(); + finalLastFrag.selfRelease = true; + } + + currentFrag.unregisterOnViewReadyListener(this); + } + }); } if (event.isFastRewind()) { @@ -544,7 +571,9 @@ private void performBackNav(NavigationController.EventC2V.OnLocationBack event) } } else { fm.popBackStack(); - logger.trace("Navigation back: On step back to location {}", currentLoc.getLocationId()); + logger.trace("Navigation back: On step back from {} to location {}", + event.getLastValue() != null ? event.getLastValue().getLocationId() : null, + currentLoc.getLocationId()); } } } diff --git a/library/android-mvc/src/main/java/com/shipdream/lib/android/mvc/view/MvcFragment.java b/library/android-mvc/src/main/java/com/shipdream/lib/android/mvc/view/MvcFragment.java index c826990..5c169ee 100644 --- a/library/android-mvc/src/main/java/com/shipdream/lib/android/mvc/view/MvcFragment.java +++ b/library/android-mvc/src/main/java/com/shipdream/lib/android/mvc/view/MvcFragment.java @@ -146,6 +146,7 @@ public String toString() { private boolean dependenciesInjected = false; boolean isStateManagedByRootDelegateFragment = false; + boolean selfRelease = true; /** * @return orientation before last orientation change. @@ -395,7 +396,9 @@ public void onDestroyView() { @Override public void onDestroy() { super.onDestroy(); - releaseDependencies(); + if (selfRelease) { + releaseDependencies(); + } eventRegister.onDestroy(); eventRegister = null; }