Skip to content

Commit 3c08b1c

Browse files
guycaguyca
authoredMay 26, 2019
Fix setStackRoot crash when called with the same id (wix#5154)
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 wix#5117
1 parent ca28810 commit 3c08b1c

File tree

7 files changed

+58
-29
lines changed

7 files changed

+58
-29
lines changed
 

‎e2e/Stack.test.js

+5
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,11 @@ describe('Stack', () => {
8080
await expect(elementById(TestIDs.STACK_SCREEN_HEADER)).toBeVisible();
8181
});
8282

83+
it('does not crash when setting the stack root to an existing component id', async () => {
84+
await elementById(TestIDs.SET_STACK_ROOT_WITH_ID_BTN).tap();
85+
await elementById(TestIDs.SET_STACK_ROOT_WITH_ID_BTN).tap();
86+
});
87+
8388
it(':ios: set stack root component should be first in stack', async () => {
8489
await elementById(TestIDs.PUSH_BTN).tap();
8590
await expect(elementByLabel('Stack Position: 1')).toBeVisible();

‎lib/android/app/src/main/java/com/reactnativenavigation/viewcontrollers/IdStack.java

+2-8
Original file line numberDiff line numberDiff line change
@@ -29,17 +29,11 @@ public void set(String id, E item, int index) {
2929
}
3030

3131
public E peek() {
32-
if (isEmpty()) {
33-
return null;
34-
}
35-
return map.get(last(deque));
32+
return isEmpty() ? null : map.get(last(deque));
3633
}
3734

3835
public E pop() {
39-
if (isEmpty()) {
40-
return null;
41-
}
42-
return map.remove(removeLast(deque));
36+
return isEmpty() ? null : map.remove(removeLast(deque));
4337
}
4438

4539
public boolean isEmpty() {

‎lib/android/app/src/main/java/com/reactnativenavigation/viewcontrollers/stack/StackController.java

+12-15
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@
1414
import com.reactnativenavigation.presentation.Presenter;
1515
import com.reactnativenavigation.presentation.StackPresenter;
1616
import com.reactnativenavigation.react.Constants;
17-
import com.reactnativenavigation.utils.CollectionUtils;
1817
import com.reactnativenavigation.utils.CommandListener;
1918
import com.reactnativenavigation.utils.CommandListenerAdapter;
2019
import com.reactnativenavigation.viewcontrollers.ChildControllersRegistry;
@@ -32,10 +31,11 @@
3231
import java.util.List;
3332

3433
import static android.view.ViewGroup.LayoutParams.MATCH_PARENT;
34+
import static com.reactnativenavigation.utils.CollectionUtils.*;
3535

3636
public class StackController extends ParentController<StackLayout> {
3737

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

182182
public void setRoot(List<ViewController> children, CommandListener listener) {
183183
animator.cancelPushAnimations();
184+
IdStack stackToDestroy = stack;
185+
stack = new IdStack<>();
184186
if (children.size() == 1) {
185-
backButtonHelper.clear(CollectionUtils.last(children));
186-
push(CollectionUtils.last(children), new CommandListenerAdapter() {
187+
backButtonHelper.clear(last(children));
188+
push(last(children), new CommandListenerAdapter() {
187189
@Override
188190
public void onSuccess(String childId) {
189-
removeChildrenBellowTop();
191+
destroyStack(stackToDestroy);
190192
listener.onSuccess(childId);
191193
}
192194
});
193195
} else {
194-
push(CollectionUtils.last(children), new CommandListenerAdapter() {
196+
push(last(children), new CommandListenerAdapter() {
195197
@Override
196198
public void onSuccess(String childId) {
197-
removeChildrenBellowTop();
199+
destroyStack(stackToDestroy);
198200
for (int i = 0; i < children.size() - 1; i++) {
199201
stack.set(children.get(i).getId(), children.get(i), i);
200202
children.get(i).setParentController(StackController.this);
@@ -210,14 +212,9 @@ public void onSuccess(String childId) {
210212
}
211213
}
212214

213-
private void removeChildrenBellowTop() {
214-
Iterator<String> iterator = stack.iterator();
215-
while (stack.size() > 1) {
216-
ViewController controller = stack.get(iterator.next());
217-
if (!stack.isTop(controller.getId())) {
218-
stack.remove(iterator, controller.getId());
219-
controller.destroy();
220-
}
215+
private void destroyStack(IdStack stack) {
216+
for (String s : (Iterable<String>) stack) {
217+
((ViewController) stack.get(s)).destroy();
221218
}
222219
}
223220

‎lib/android/app/src/test/java/com/reactnativenavigation/viewcontrollers/stack/StackControllerTest.java

+24-3
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,11 @@
4242
import org.json.JSONException;
4343
import org.json.JSONObject;
4444
import org.junit.Test;
45-
import org.mockito.*;
45+
import org.mockito.ArgumentCaptor;
46+
import org.mockito.InOrder;
47+
import org.mockito.Mockito;
48+
import org.robolectric.Robolectric;
49+
import org.robolectric.shadows.ShadowLooper;
4650

4751
import java.util.ArrayList;
4852
import java.util.Arrays;
@@ -65,6 +69,7 @@ public class StackControllerTest extends BaseTest {
6569
private ChildControllersRegistry childRegistry;
6670
private StackController uut;
6771
private ViewController child1;
72+
private ViewController child1a;
6873
private ViewController child2;
6974
private ViewController child3;
7075
private ViewController child4;
@@ -84,6 +89,7 @@ public void beforeEach() {
8489
renderChecker = spy(new RenderChecker());
8590
presenter = spy(new StackPresenter(activity, new TitleBarReactViewCreatorMock(), new TopBarBackgroundViewCreatorMock(), new TopBarButtonCreatorMock(), ImageLoaderMock.mock(), renderChecker, new Options()));
8691
child1 = spy(new SimpleViewController(activity, childRegistry, "child1", new Options()));
92+
child1a = spy(new SimpleViewController(activity, childRegistry, "child1", new Options()));
8793
child2 = spy(new SimpleViewController(activity, childRegistry, "child2", new Options()));
8894
child3 = spy(new SimpleViewController(activity, childRegistry, "child3", new Options()));
8995
child4 = spy(new SimpleViewController(activity, childRegistry, "child4", new Options()));
@@ -257,13 +263,16 @@ public void onSuccess(String childId) {
257263

258264
@Test
259265
public void setRoot_multipleChildren() {
266+
Robolectric.getForegroundThreadScheduler().pause();
267+
260268
activity.setContentView(uut.getView());
261269
disablePushAnimation(child1, child2, child3, child4);
262270
disablePopAnimation(child4);
263271

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

276285
assertThat(uut.getCurrentChild()).isEqualTo(child4);
277286
uut.pop(Options.EMPTY, new CommandListenerAdapter());
287+
ShadowLooper.idleMainLooper();
278288
assertThat(uut.getTopBar().getTitleBar().getNavigationIcon()).isNull();
279289
assertThat(uut.getCurrentChild()).isEqualTo(child3);
280290
}
@@ -286,14 +296,25 @@ public void setRoot_doesNotCrashWhenCalledInQuickSuccession() {
286296
disablePushAnimation(child1);
287297
uut.setRoot(Collections.singletonList(child1), new CommandListenerAdapter());
288298

299+
ViewGroup c2View = child2.getView();
300+
ViewGroup c3View = child3.getView();
289301
uut.setRoot(Collections.singletonList(child2), new CommandListenerAdapter());
290302
uut.setRoot(Collections.singletonList(child3), new CommandListenerAdapter());
291-
animator.endPushAnimation(child2.getView());
292-
animator.endPushAnimation(child3.getView());
303+
animator.endPushAnimation(c2View);
304+
animator.endPushAnimation(c3View);
293305

294306
assertContainsOnlyId(child3.getId());
295307
}
296308

309+
@Test
310+
public void setRoot_doesNotCrashWhenCalledWithSameId() {
311+
disablePushAnimation(child1, child1a);
312+
uut.setRoot(Collections.singletonList(child1), new CommandListenerAdapter());
313+
uut.setRoot(Collections.singletonList(child1a), new CommandListenerAdapter());
314+
315+
assertContainsOnlyId(child1a.getId());
316+
}
317+
297318
@Test
298319
public synchronized void pop() {
299320
disablePushAnimation(child1, child2);

‎playground/src/screens/StackScreen.js

+13-2
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ const Root = require('../components/Root');
33
const Button = require('../components/Button');
44
const Screens = require('./Screens');
55
const Navigation = require('../services/Navigation');
6-
const {stack, component} = require('../commons/Layouts');
6+
const { stack, component } = require('../commons/Layouts');
77
const {
88
PUSH_BTN,
99
STACK_SCREEN_HEADER,
@@ -12,7 +12,8 @@ const {
1212
PUSH_CUSTOM_BACK_BTN,
1313
CUSTOM_BACK_BTN,
1414
SEARCH_BTN,
15-
SET_STACK_ROOT_BTN
15+
SET_STACK_ROOT_BTN,
16+
SET_STACK_ROOT_WITH_ID_BTN
1617
} = require('../testIDs');
1718

1819
class StackScreen extends React.Component {
@@ -39,6 +40,7 @@ class StackScreen extends React.Component {
3940
<Button label='Pop None Existent Screen' testID={POP_NONE_EXISTENT_SCREEN_BTN} onPress={this.popNoneExistent} />
4041
<Button label='Push Custom Back Button' testID={PUSH_CUSTOM_BACK_BTN} onPress={this.pushCustomBackButton} />
4142
<Button label='Set Stack Root' testID={SET_STACK_ROOT_BTN} onPress={this.setStackRoot} />
43+
<Button label='Set Stack Root With ID' testID={SET_STACK_ROOT_WITH_ID_BTN} onPress={this.setStackRootWithId} />
4244
<Button label='Search' testID={SEARCH_BTN} onPress={this.search} />
4345
</Root>
4446
);
@@ -73,6 +75,15 @@ class StackScreen extends React.Component {
7375
component(Screens.Pushed, { topBar: { title: { text: 'Screen A' } } }),
7476
component(Screens.Pushed, { topBar: { title: { text: 'Screen B' } } }),
7577
]));
78+
79+
setStackRootWithId = () => Navigation.setStackRoot(this,
80+
{
81+
component: {
82+
id: 'StackRootWithId',
83+
name: Screens.Stack
84+
}
85+
},
86+
);
7687
}
7788

7889
module.exports = StackScreen;

‎playground/src/testIDs.js

+1
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@ module.exports = {
8383
DISMISS_BTN: 'DISMISS_BTN',
8484
SEARCH_BTN: 'SEARCH_BTN',
8585
SET_STACK_ROOT_BTN: 'SET_STACK_ROOT_BTN',
86+
SET_STACK_ROOT_WITH_ID_BTN: 'SET_STACK_ROOT_WITH_ID_BTN',
8687

8788
// Buttons
8889
TAB_BASED_APP_BUTTON: `TAB_BASED_APP_BUTTON`,

‎scripts/test-e2e.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -19,5 +19,5 @@ function run() {
1919
if (!skipBuild) {
2020
exec.execSync(`detox build --configuration ${configuration}`);
2121
}
22-
exec.execSync(`detox test --configuration ${configuration} ${headless$} ${!android ? `-w ${workers}` : ``} --loglevel trace`); //-f "ScreenStyle.test.js" --loglevel trace
22+
exec.execSync(`detox test --configuration ${configuration} ${headless$} ${!android ? `-w ${workers}` : ``}`); //-f "ScreenStyle.test.js" --loglevel trace
2323
}

0 commit comments

Comments
 (0)
Failed to load comments.