Skip to content

Commit

Permalink
Fix setStackRoot crash when called with the same id (#5154)
Browse files Browse the repository at this point in the history 
When a Stack’s root was set with an id of one of the Stack’s current children, there was a crash
since the wrong elements were removed from the stack.

This commit fixes this by creating a new stack when setStackRoot is called, and destroying all ViewControllers from the previous Stack.

Fixes #5117
  • Loading branch information
@guyca
guyca authored May 26, 2019
1 parent ca28810 commit 3c08b1c
Show file tree
Hide file tree
Showing 7 changed files with 58 additions and 29 deletions.
5 changes: 5 additions & 0 deletions e2e/Stack.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,11 @@ describe('Stack', () => {
await expect(elementById(TestIDs.STACK_SCREEN_HEADER)).toBeVisible();
});

it('does not crash when setting the stack root to an existing component id', async () => {
await elementById(TestIDs.SET_STACK_ROOT_WITH_ID_BTN).tap();
await elementById(TestIDs.SET_STACK_ROOT_WITH_ID_BTN).tap();
});

it(':ios: set stack root component should be first in stack', async () => {
await elementById(TestIDs.PUSH_BTN).tap();
await expect(elementByLabel('Stack Position: 1')).toBeVisible();
Expand Down
10 changes: 2 additions & 8 deletions lib/android/app/src/main/java/com/reactnativenavigation/viewcontrollers/IdStack.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,17 +29,11 @@ public void set(String id, E item, int index) {
}

public E peek() {
if (isEmpty()) {
return null;
}
return map.get(last(deque));
return isEmpty() ? null : map.get(last(deque));
}

public E pop() {
if (isEmpty()) {
return null;
}
return map.remove(removeLast(deque));
return isEmpty() ? null : map.remove(removeLast(deque));
}

public boolean isEmpty() {
Expand Down
27 changes: 12 additions & 15 deletions ...id/app/src/main/java/com/reactnativenavigation/viewcontrollers/stack/StackController.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
import com.reactnativenavigation.presentation.Presenter;
import com.reactnativenavigation.presentation.StackPresenter;
import com.reactnativenavigation.react.Constants;
import com.reactnativenavigation.utils.CollectionUtils;
import com.reactnativenavigation.utils.CommandListener;
import com.reactnativenavigation.utils.CommandListenerAdapter;
import com.reactnativenavigation.viewcontrollers.ChildControllersRegistry;
Expand All @@ -32,10 +31,11 @@
import java.util.List;

import static android.view.ViewGroup.LayoutParams.MATCH_PARENT;
import static com.reactnativenavigation.utils.CollectionUtils.*;

public class StackController extends ParentController<StackLayout> {

private final IdStack<ViewController> stack = new IdStack<>();
private IdStack<ViewController> stack = new IdStack<>();
private final NavigationAnimator animator;
private TopBarController topBarController;
private BackButtonHelper backButtonHelper;
Expand Down Expand Up @@ -181,20 +181,22 @@ private void addChildToStack(ViewController child, View view, Options resolvedOp

public void setRoot(List<ViewController> children, CommandListener listener) {
animator.cancelPushAnimations();
IdStack stackToDestroy = stack;
stack = new IdStack<>();
if (children.size() == 1) {
backButtonHelper.clear(CollectionUtils.last(children));
push(CollectionUtils.last(children), new CommandListenerAdapter() {
backButtonHelper.clear(last(children));
push(last(children), new CommandListenerAdapter() {
@Override
public void onSuccess(String childId) {
removeChildrenBellowTop();
destroyStack(stackToDestroy);
listener.onSuccess(childId);
}
});
} else {
push(CollectionUtils.last(children), new CommandListenerAdapter() {
push(last(children), new CommandListenerAdapter() {
@Override
public void onSuccess(String childId) {
removeChildrenBellowTop();
destroyStack(stackToDestroy);
for (int i = 0; i < children.size() - 1; i++) {
stack.set(children.get(i).getId(), children.get(i), i);
children.get(i).setParentController(StackController.this);
Expand All @@ -210,14 +212,9 @@ public void onSuccess(String childId) {
}
}

private void removeChildrenBellowTop() {
Iterator<String> iterator = stack.iterator();
while (stack.size() > 1) {
ViewController controller = stack.get(iterator.next());
if (!stack.isTop(controller.getId())) {
stack.remove(iterator, controller.getId());
controller.destroy();
}
private void destroyStack(IdStack stack) {
for (String s : (Iterable<String>) stack) {
((ViewController) stack.get(s)).destroy();
}
}

Expand Down
27 changes: 24 additions & 3 deletions ...pp/src/test/java/com/reactnativenavigation/viewcontrollers/stack/StackControllerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,11 @@
import org.json.JSONException;
import org.json.JSONObject;
import org.junit.Test;
import org.mockito.*;
import org.mockito.ArgumentCaptor;
import org.mockito.InOrder;
import org.mockito.Mockito;
import org.robolectric.Robolectric;
import org.robolectric.shadows.ShadowLooper;

import java.util.ArrayList;
import java.util.Arrays;
Expand All @@ -65,6 +69,7 @@ public class StackControllerTest extends BaseTest {
private ChildControllersRegistry childRegistry;
private StackController uut;
private ViewController child1;
private ViewController child1a;
private ViewController child2;
private ViewController child3;
private ViewController child4;
Expand All @@ -84,6 +89,7 @@ public void beforeEach() {
renderChecker = spy(new RenderChecker());
presenter = spy(new StackPresenter(activity, new TitleBarReactViewCreatorMock(), new TopBarBackgroundViewCreatorMock(), new TopBarButtonCreatorMock(), ImageLoaderMock.mock(), renderChecker, new Options()));
child1 = spy(new SimpleViewController(activity, childRegistry, "child1", new Options()));
child1a = spy(new SimpleViewController(activity, childRegistry, "child1", new Options()));
child2 = spy(new SimpleViewController(activity, childRegistry, "child2", new Options()));
child3 = spy(new SimpleViewController(activity, childRegistry, "child3", new Options()));
child4 = spy(new SimpleViewController(activity, childRegistry, "child4", new Options()));
Expand Down Expand Up @@ -257,13 +263,16 @@ public void onSuccess(String childId) {

@Test
public void setRoot_multipleChildren() {
Robolectric.getForegroundThreadScheduler().pause();

activity.setContentView(uut.getView());
disablePushAnimation(child1, child2, child3, child4);
disablePopAnimation(child4);

assertThat(uut.isEmpty()).isTrue();
uut.push(child1, new CommandListenerAdapter());
uut.push(child2, new CommandListenerAdapter());
ShadowLooper.idleMainLooper();
assertThat(uut.getTopBar().getTitleBar().getNavigationIcon()).isNotNull();
uut.setRoot(Arrays.asList(child3, child4), new CommandListenerAdapter() {
@Override
Expand All @@ -275,6 +284,7 @@ public void onSuccess(String childId) {

assertThat(uut.getCurrentChild()).isEqualTo(child4);
uut.pop(Options.EMPTY, new CommandListenerAdapter());
ShadowLooper.idleMainLooper();
assertThat(uut.getTopBar().getTitleBar().getNavigationIcon()).isNull();
assertThat(uut.getCurrentChild()).isEqualTo(child3);
}
Expand All @@ -286,14 +296,25 @@ public void setRoot_doesNotCrashWhenCalledInQuickSuccession() {
disablePushAnimation(child1);
uut.setRoot(Collections.singletonList(child1), new CommandListenerAdapter());

ViewGroup c2View = child2.getView();
ViewGroup c3View = child3.getView();
uut.setRoot(Collections.singletonList(child2), new CommandListenerAdapter());
uut.setRoot(Collections.singletonList(child3), new CommandListenerAdapter());
animator.endPushAnimation(child2.getView());
animator.endPushAnimation(child3.getView());
animator.endPushAnimation(c2View);
animator.endPushAnimation(c3View);

assertContainsOnlyId(child3.getId());
}

@Test
public void setRoot_doesNotCrashWhenCalledWithSameId() {
disablePushAnimation(child1, child1a);
uut.setRoot(Collections.singletonList(child1), new CommandListenerAdapter());
uut.setRoot(Collections.singletonList(child1a), new CommandListenerAdapter());

assertContainsOnlyId(child1a.getId());
}

@Test
public synchronized void pop() {
disablePushAnimation(child1, child2);
Expand Down
15 changes: 13 additions & 2 deletions playground/src/screens/StackScreen.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ const Root = require('../components/Root');
const Button = require('../components/Button');
const Screens = require('./Screens');
const Navigation = require('../services/Navigation');
const {stack, component} = require('../commons/Layouts');
const { stack, component } = require('../commons/Layouts');
const {
PUSH_BTN,
STACK_SCREEN_HEADER,
Expand All @@ -12,7 +12,8 @@ const {
PUSH_CUSTOM_BACK_BTN,
CUSTOM_BACK_BTN,
SEARCH_BTN,
SET_STACK_ROOT_BTN
SET_STACK_ROOT_BTN,
SET_STACK_ROOT_WITH_ID_BTN
} = require('../testIDs');

class StackScreen extends React.Component {
Expand All @@ -39,6 +40,7 @@ class StackScreen extends React.Component {
<Button label='Pop None Existent Screen' testID={POP_NONE_EXISTENT_SCREEN_BTN} onPress={this.popNoneExistent} />
<Button label='Push Custom Back Button' testID={PUSH_CUSTOM_BACK_BTN} onPress={this.pushCustomBackButton} />
<Button label='Set Stack Root' testID={SET_STACK_ROOT_BTN} onPress={this.setStackRoot} />
<Button label='Set Stack Root With ID' testID={SET_STACK_ROOT_WITH_ID_BTN} onPress={this.setStackRootWithId} />
<Button label='Search' testID={SEARCH_BTN} onPress={this.search} />
</Root>
);
Expand Down Expand Up @@ -73,6 +75,15 @@ class StackScreen extends React.Component {
component(Screens.Pushed, { topBar: { title: { text: 'Screen A' } } }),
component(Screens.Pushed, { topBar: { title: { text: 'Screen B' } } }),
]));

setStackRootWithId = () => Navigation.setStackRoot(this,
{
component: {
id: 'StackRootWithId',
name: Screens.Stack
}
},
);
}

module.exports = StackScreen;
1 change: 1 addition & 0 deletions playground/src/testIDs.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ module.exports = {
DISMISS_BTN: 'DISMISS_BTN',
SEARCH_BTN: 'SEARCH_BTN',
SET_STACK_ROOT_BTN: 'SET_STACK_ROOT_BTN',
SET_STACK_ROOT_WITH_ID_BTN: 'SET_STACK_ROOT_WITH_ID_BTN',

// Buttons
TAB_BASED_APP_BUTTON: `TAB_BASED_APP_BUTTON`,
Expand Down
2 changes: 1 addition & 1 deletion scripts/test-e2e.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,5 @@ function run() {
if (!skipBuild) {
exec.execSync(`detox build --configuration ${configuration}`);
}
exec.execSync(`detox test --configuration ${configuration} ${headless$} ${!android ? `-w ${workers}` : ``} --loglevel trace`); //-f "ScreenStyle.test.js" --loglevel trace
exec.execSync(`detox test --configuration ${configuration} ${headless$} ${!android ? `-w ${workers}` : ``}`); //-f "ScreenStyle.test.js" --loglevel trace
}

0 comments on commit 3c08b1c

Please sign in to comment.