From cea829ae886b465ddf90529063d1d8ad03776bfb Mon Sep 17 00:00:00 2001 From: Aleksey Dolgiy Date: Wed, 19 Feb 2020 17:37:25 +0300 Subject: [PATCH 1/3] Fix NPE when localStackCopy.size() == 0 and FragmentFactory is used --- .../android/support/SupportAppNavigator.java | 45 +++++++++++++------ 1 file changed, 31 insertions(+), 14 deletions(-) diff --git a/library/src/main/java/ru/terrakok/cicerone/android/support/SupportAppNavigator.java b/library/src/main/java/ru/terrakok/cicerone/android/support/SupportAppNavigator.java index 6f8672d..ecf2b85 100644 --- a/library/src/main/java/ru/terrakok/cicerone/android/support/SupportAppNavigator.java +++ b/library/src/main/java/ru/terrakok/cicerone/android/support/SupportAppNavigator.java @@ -3,17 +3,24 @@ import android.app.Activity; import android.content.Intent; import android.os.Bundle; + import androidx.fragment.app.Fragment; import androidx.fragment.app.FragmentActivity; import androidx.fragment.app.FragmentManager; import androidx.fragment.app.FragmentTransaction; + import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; -import ru.terrakok.cicerone.Navigator; -import ru.terrakok.cicerone.commands.*; import java.util.LinkedList; +import ru.terrakok.cicerone.Navigator; +import ru.terrakok.cicerone.commands.Back; +import ru.terrakok.cicerone.commands.BackTo; +import ru.terrakok.cicerone.commands.Command; +import ru.terrakok.cicerone.commands.Forward; +import ru.terrakok.cicerone.commands.Replace; + /** * Navigator implementation for launch fragments and activities.
* Feature {@link BackTo} works only for fragments.
@@ -164,14 +171,8 @@ protected void fragmentReplace(@NotNull Replace command) { fragmentTransaction ); - if (fragmentParams != null) { - fragmentTransaction.replace( - containerId, - fragmentParams.getFragmentClass(), - fragmentParams.getArguments()); - } else { - fragmentTransaction.replace(containerId, fragment); - } + fragmentReplaceInternal(fragmentTransaction, fragmentParams, fragment); + fragmentTransaction .addToBackStack(screen.getScreenKey()) .commit(); @@ -187,9 +188,25 @@ protected void fragmentReplace(@NotNull Replace command) { fragmentTransaction ); - fragmentTransaction - .replace(containerId, fragment) - .commit(); + fragmentReplaceInternal(fragmentTransaction, fragmentParams, fragment); + + fragmentTransaction.commit(); + } + } + + private void fragmentReplaceInternal( + @NotNull FragmentTransaction transaction, + @Nullable FragmentParams params, + @Nullable Fragment fragment + ) { + if (params != null) { + transaction.replace( + containerId, + params.getFragmentClass(), + params.getArguments() + ); + } else { + transaction.replace(containerId, fragment); } } @@ -307,7 +324,7 @@ protected void errorWhileCreatingScreen(@NotNull SupportAppScreen screen) { * Override this method if you want to handle apply command error. * * @param command command - * @param error error + * @param error error */ protected void errorOnApplyCommand( @NotNull Command command, From d08e6b1afe0583dc807cf493c706b5d9c33e1c27 Mon Sep 17 00:00:00 2001 From: Aleksey Dolgiy Date: Wed, 19 Feb 2020 20:46:31 +0300 Subject: [PATCH 2/3] Code Review: Use internal replace for fragmentForward(Forward) and add explicit exception with null checks --- .../android/support/SupportAppNavigator.java | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/library/src/main/java/ru/terrakok/cicerone/android/support/SupportAppNavigator.java b/library/src/main/java/ru/terrakok/cicerone/android/support/SupportAppNavigator.java index ecf2b85..b4870a7 100644 --- a/library/src/main/java/ru/terrakok/cicerone/android/support/SupportAppNavigator.java +++ b/library/src/main/java/ru/terrakok/cicerone/android/support/SupportAppNavigator.java @@ -113,15 +113,12 @@ protected void fragmentForward(@NotNull Forward command) { fragment, fragmentTransaction); - if (fragmentParams != null) { - fragmentTransaction.replace(containerId, fragmentParams.getFragmentClass(), fragmentParams.getArguments()); - } else { - fragmentTransaction.replace(containerId, fragment); - } + fragmentReplaceInternal(fragmentTransaction, screen, fragmentParams, fragment); fragmentTransaction .addToBackStack(screen.getScreenKey()) .commit(); + localStackCopy.add(screen.getScreenKey()); } @@ -171,11 +168,12 @@ protected void fragmentReplace(@NotNull Replace command) { fragmentTransaction ); - fragmentReplaceInternal(fragmentTransaction, fragmentParams, fragment); + fragmentReplaceInternal(fragmentTransaction, screen, fragmentParams, fragment); fragmentTransaction .addToBackStack(screen.getScreenKey()) .commit(); + localStackCopy.add(screen.getScreenKey()); } else { @@ -188,7 +186,7 @@ protected void fragmentReplace(@NotNull Replace command) { fragmentTransaction ); - fragmentReplaceInternal(fragmentTransaction, fragmentParams, fragment); + fragmentReplaceInternal(fragmentTransaction, screen, fragmentParams, fragment); fragmentTransaction.commit(); } @@ -196,6 +194,7 @@ protected void fragmentReplace(@NotNull Replace command) { private void fragmentReplaceInternal( @NotNull FragmentTransaction transaction, + @NotNull SupportAppScreen screen, @Nullable FragmentParams params, @Nullable Fragment fragment ) { @@ -205,8 +204,11 @@ private void fragmentReplaceInternal( params.getFragmentClass(), params.getArguments() ); - } else { + } else if (fragment != null) { transaction.replace(containerId, fragment); + } else { + throw new IllegalArgumentException("Either 'params' or 'fragment' shouldn't " + + "be null for " + screen.getScreenKey()); } } From 05a8ec8f44e6fec14b7167c3b4b71ca5751b7edc Mon Sep 17 00:00:00 2001 From: Aleksey Dolgiy Date: Thu, 20 Feb 2020 10:29:18 +0300 Subject: [PATCH 3/3] Extract 'forward' to reuse in replace when localStackCopy.size > 0 --- .../android/support/SupportAppNavigator.java | 59 +++++++++---------- 1 file changed, 27 insertions(+), 32 deletions(-) diff --git a/library/src/main/java/ru/terrakok/cicerone/android/support/SupportAppNavigator.java b/library/src/main/java/ru/terrakok/cicerone/android/support/SupportAppNavigator.java index b4870a7..3aa48c2 100644 --- a/library/src/main/java/ru/terrakok/cicerone/android/support/SupportAppNavigator.java +++ b/library/src/main/java/ru/terrakok/cicerone/android/support/SupportAppNavigator.java @@ -105,21 +105,7 @@ protected void fragmentForward(@NotNull Forward command) { FragmentParams fragmentParams = screen.getFragmentParams(); Fragment fragment = fragmentParams == null ? createFragment(screen) : null; - FragmentTransaction fragmentTransaction = fragmentManager.beginTransaction(); - - setupFragmentTransaction( - command, - fragmentManager.findFragmentById(containerId), - fragment, - fragmentTransaction); - - fragmentReplaceInternal(fragmentTransaction, screen, fragmentParams, fragment); - - fragmentTransaction - .addToBackStack(screen.getScreenKey()) - .commit(); - - localStackCopy.add(screen.getScreenKey()); + forwardFragmentInternal(command, screen, fragmentParams, fragment); } protected void fragmentBack() { @@ -159,6 +145,9 @@ protected void fragmentReplace(@NotNull Replace command) { fragmentManager.popBackStack(); localStackCopy.removeLast(); + forwardFragmentInternal(command, screen, fragmentParams, fragment); + + } else { FragmentTransaction fragmentTransaction = fragmentManager.beginTransaction(); setupFragmentTransaction( @@ -168,31 +157,37 @@ protected void fragmentReplace(@NotNull Replace command) { fragmentTransaction ); - fragmentReplaceInternal(fragmentTransaction, screen, fragmentParams, fragment); + replaceFragmentInternal(fragmentTransaction, screen, fragmentParams, fragment); - fragmentTransaction - .addToBackStack(screen.getScreenKey()) - .commit(); + fragmentTransaction.commit(); + } + } - localStackCopy.add(screen.getScreenKey()); + private void forwardFragmentInternal( + @NotNull Command command, + @NotNull SupportAppScreen screen, + @Nullable FragmentParams fragmentParams, + @Nullable Fragment fragment + ) { + FragmentTransaction fragmentTransaction = fragmentManager.beginTransaction(); - } else { - FragmentTransaction fragmentTransaction = fragmentManager.beginTransaction(); + setupFragmentTransaction( + command, + fragmentManager.findFragmentById(containerId), + fragment, + fragmentTransaction + ); - setupFragmentTransaction( - command, - fragmentManager.findFragmentById(containerId), - fragment, - fragmentTransaction - ); + replaceFragmentInternal(fragmentTransaction, screen, fragmentParams, fragment); - fragmentReplaceInternal(fragmentTransaction, screen, fragmentParams, fragment); + fragmentTransaction + .addToBackStack(screen.getScreenKey()) + .commit(); - fragmentTransaction.commit(); - } + localStackCopy.add(screen.getScreenKey()); } - private void fragmentReplaceInternal( + private void replaceFragmentInternal( @NotNull FragmentTransaction transaction, @NotNull SupportAppScreen screen, @Nullable FragmentParams params,