fix(renderer): native elements could not be moved (fixes TodoMVC sample)

Author: marclaval

src/renderer/native_command.ts

@@ -135,8 +135,18 @@ export class NativeCommandAttachAfter extends NativeCommand {
  private _attach(wrapper: ReactNativeWrapper, node: Node, ancestor: Node, counters: Map<Node, number>, baseNativeIndex: number): void {
  var shift = counters.get(this.anchor) || 0;
  var nativeIndex = baseNativeIndex + shift;
- wrapper.manageChildren(ancestor.nativeTag, null, null, [node.nativeTag], [nativeIndex], null);
- ancestor.nativeChildren.splice(nativeIndex, 0, node.nativeTag);
+ if (!node.toBeMoved) {
+ wrapper.manageChildren(ancestor.nativeTag, null, null, [node.nativeTag], [nativeIndex], null);
+ ancestor.nativeChildren.splice(nativeIndex, 0, node.nativeTag);
+ } else {
+ var currentIndex = ancestor.nativeChildren.indexOf(node.nativeTag);
+ if (currentIndex != nativeIndex) {
+ wrapper.manageChildren(ancestor.nativeTag, [currentIndex], [nativeIndex], null, null, null);
+ ancestor.nativeChildren.splice(currentIndex, 1);
+ ancestor.nativeChildren.splice(nativeIndex, 0, node.nativeTag);
+ }
+ node.toBeMoved = false;
+ }
  counters.set(this.anchor, shift + 1);
  }
 }
@@ -148,14 +158,18 @@ export class NativeCommandDetach extends NativeCommand {
 
  execute(wrapper: ReactNativeWrapper) {
  var parent = this.target.parent;
- var index = parent.children.indexOf(this.target);
- parent.children.splice(index, 1);
  var toDetach = this.target.nativeTag > -1 ? this.target : (this.target.isVirtual ? this.target.getFirstCreatedChild() : null);
  if (toDetach) {
- var nativeIndex = parent.nativeChildren.indexOf(toDetach.nativeTag);
- parent.nativeChildren.splice(nativeIndex, 1);
- wrapper.manageChildren(parent.nativeTag, null, null, null, null, [nativeIndex]);
- toDetach.destroyNative();
+ if (toDetach.toBeDestroyed) {
+ var nativeIndex = parent.nativeChildren.indexOf(toDetach.nativeTag);
+ parent.nativeChildren.splice(nativeIndex, 1);
+ wrapper.manageChildren(parent.nativeTag, null, null, null, null, [nativeIndex]);
+ toDetach.destroyNative();
+ }
+ else {
+ //Node is being moved
+ this.target.toBeMoved = true;
+ }
  }
  }
 }

src/renderer/node.ts

@@ -10,6 +10,8 @@ export abstract class Node {
  public tagName: string = "";
  public properties: {[s: string]: any } = {};
  public isVirtual: boolean = false;
+ public toBeDestroyed: boolean = false;
+ public toBeMoved: boolean = false;
 
  public nativeTag: number = -1;
  public isCreated: boolean = false;
@@ -121,6 +123,7 @@ export abstract class Node {
  nodeMap.delete(this.nativeTag);
  this.nativeTag = -1;
  this.nativeChildren = [];
+ this.toBeDestroyed = this.toBeMoved = false;
  for (var i = 0; i < this.children.length; i++) {
  this.children[i].destroyNative();
  }

src/renderer/renderer.ts

@@ -192,12 +192,18 @@ export class ReactNativeRenderer implements Renderer {
  detachView(viewRootNodes: Node[]): void {
  for (var i = 0; i < viewRootNodes.length; i++) {
  var node = viewRootNodes[i];
+ var parent = node.parent;
+ var index = parent.children.indexOf(node);
+ parent.children.splice(index, 1);
  this._rootRenderer.addDetachCommand(node);
  }
  }
 
- destroyView(hostElement:any, viewAllNodes:any[]):any {
- //TODO: Nothing to do, detachView took care of it. Can it be improved to avoid destruction and creation?
+ destroyView(hostElement: any, viewAllNodes: Node[]): void {
+ for (var i = 0; i < viewAllNodes.length; i++) {
+ var node = viewAllNodes[i];
+ node.toBeDestroyed = true;
+ }
  }
 
  listen(renderElement: Node, name: string, callback: Function): Function {

src/wrapper/wrapper_mock.ts

@@ -1,5 +1,12 @@
 import {ReactNativeWrapper, overridePlatform} from "./wrapper";
 
+class Command {
+ constructor(public name: string, public target: number, public details: string) { }
+ toString(): string {
+ return `${this.name}+${this.target}+${this.details}`;
+ }
+}
+
 export class MockReactNativeWrapper extends ReactNativeWrapper {
  commandLogs: Array<Command>;
  root: NativeElement;
@@ -46,34 +53,44 @@ export class MockReactNativeWrapper extends ReactNativeWrapper {
  // moveTo and addAt are both relative to the final state of the View's children.
  manageChildren(parentTag: number, moveFrom: Array<number>, moveTo: Array<number>, addTags: Array<number>, addAt: Array<number>, removeFrom: Array<number>) {
  var parentElement = this.nativeElementMap.get(parentTag);
- var toBeDeleted = removeFrom || [];
+ var toBeDeleted = [];
  var toBeAdded: Array<any> = [];
- var toBeMoved: Array<any> = [];
- if (moveFrom && moveTo && moveFrom.length == moveTo.length) {
- toBeDeleted = toBeDeleted.concat(moveFrom).sort();
+ if (moveFrom && moveTo && moveFrom.length != moveTo.length) {
+ throw new Error(`manageChildren - MOVE - Invalid lengths: ${moveFrom.length} vs ${moveTo.length}`);
+ }
+ if (addTags && addAt && addTags.length != addAt.length) {
+ throw new Error(`manageChildren - MOVE - Invalid lengths: ${addTags.length} vs ${addAt.length}`);
+ }
+ //Detach commands
+ if (removeFrom) {
+ for (var i = 0; i < removeFrom.length; i++) {
+ var index = removeFrom[removeFrom.length - i - 1];
+ toBeDeleted.push(index);
+ this.commandLogs.push(new Command('DETACH', parentTag, '' + index));
+ }
+ }
+ //Move commands
+ if (moveFrom) {
  for (var i = 0; i < moveFrom.length; i++) {
  var tag = parentElement.children[moveFrom[i]].tag;
+ toBeDeleted.push(moveFrom[i]);
  toBeAdded.push({index: moveTo[i], tag: tag});
- toBeMoved.push(tag);
  }
+ this.commandLogs.push(new Command('MOVE', parentTag, '' + moveFrom.join(',') + '+' + moveTo.join(',')));
  }
- if (addTags && addAt && addTags.length == addAt.length) {
+ //Attach commands
+ if (addTags) {
  for (var i = 0; i < addTags.length; i++) {
  toBeAdded.push({index: addAt[i], tag: addTags[i]});
+ this.commandLogs.push(new Command('ATTACH', parentTag, addTags[i] + '+' + addAt[i]));
  }
  }
- //Detach
+ //Update data structure
+ toBeDeleted.sort();
+ toBeAdded.sort((a, b) => { return a.index - b.index});
  for (var i = 0; i < toBeDeleted.length; i++) {
- var index = toBeDeleted[toBeDeleted.length - i - 1];
- var tag = parentElement.children[index].tag;
- parentElement.children.splice(index, 1);
- if (toBeMoved.indexOf(tag) == -1) {
- this.nativeElementMap.delete(tag);
- }
- this.commandLogs.push(new Command('DETACH', parentTag, '' + index));
+ parentElement.children.splice(toBeDeleted[toBeDeleted.length - i - 1], 1);
  }
- //Attach
- toBeAdded.sort((a, b) => { return a.index - b.index});
  for (var i = 0; i < toBeAdded.length; i++) {
  var item = toBeAdded[i];
  var element = this.nativeElementMap.get(item.tag);
@@ -83,8 +100,6 @@ export class MockReactNativeWrapper extends ReactNativeWrapper {
  } else {
  throw new Error(`manageChildren - ATTACH - Invalid index ${item.index}, size is ${parentElement.children.length}`);
  }
-
- this.commandLogs.push(new Command('ATTACH', parentTag, item.tag + '+' + item.index));
  }
  }
 
@@ -158,13 +173,6 @@ export class MockReactNativeWrapper extends ReactNativeWrapper {
  }
 }
 
-class Command {
- constructor(public name: string, public target: number, public details: string) { }
- toString(): string {
- return `${this.name}+${this.target}+${this.details}`;
- }
-}
-
 export class NativeElement {
  name: string;
  tag: number;

test/renderer/element_spec.ts

@@ -179,10 +179,17 @@ describe('Element', () => {
  rootRenderer.executeCommands();
  expect(mock.nativeElementMap.get(1).children[0].children.map((a: NativeElement) => a.children[0].properties['text']).join(',')).toEqual('1,2,3');
 
+ mock.clearLogs();
+ fixture.debugElement.componentInstance.a.splice(1, 1);
+ fixture.detectChanges();
+ rootRenderer.executeCommands();
+ expect(mock.commandLogs.toString()).toEqual('DETACH+2+1'); //MOVE+2+1+1 is removed for optimization
+ expect(mock.nativeElementMap.get(1).children[0].children.map((a: NativeElement) => a.children[0].properties['text']).join(',')).toEqual('1,3');
+
  fixture.debugElement.componentInstance.a.pop();
  fixture.detectChanges();
  rootRenderer.executeCommands();
- expect(mock.nativeElementMap.get(1).children[0].children.map((a: NativeElement) => a.children[0].properties['text']).join(',')).toEqual('1,2');
+ expect(mock.nativeElementMap.get(1).children[0].children.map((a: NativeElement) => a.children[0].properties['text']).join(',')).toEqual('1');
 
  fixture.debugElement.componentInstance.a = [];
  fixture.detectChanges();

test/wrapper/wrapper_mock_spec.ts

@@ -70,11 +70,9 @@ describe('MockReactNativeWrapper', () => {
  mock.manageChildren(1, null, null, null, null, [0]);
  expect(mock.commandLogs.length).toEqual(1);
  expect(mock.commandLogs.toString()).toEqual('DETACH+1+0');
- var element1 = mock.nativeElementMap.get(2);
- var element2 = mock.nativeElementMap.get(3);
- expect(element1).toEqual(undefined);
- expect(element2.parent).toEqual(mock.root);
- expect(mock.root.children).toEqual([element2]);
+ var element = mock.nativeElementMap.get(3);
+ expect(element.parent).toEqual(mock.root);
+ expect(mock.root.children).toEqual([element]);
  });
 
  it('should move a native element', () => {
@@ -85,8 +83,8 @@ describe('MockReactNativeWrapper', () => {
  mock.manageChildren(1, null, null, [2,3,4], [0,1,2], null);
  mock.clearLogs();
  mock.manageChildren(1, [1], [2], null, null, null);
- expect(mock.commandLogs.length).toEqual(2);
- expect(mock.commandLogs.toString()).toEqual('DETACH+1+1,ATTACH+1+3+2');
+ expect(mock.commandLogs.length).toEqual(1);
+ expect(mock.commandLogs.toString()).toEqual('MOVE+1+1+2');
  var element = mock.nativeElementMap.get(3);
  expect(element.parent).toEqual(mock.root);
  expect(mock.root.children[2]).toEqual(element);
@@ -102,8 +100,8 @@ describe('MockReactNativeWrapper', () => {
  mock.createView('RCTView', 1, {});
  mock.clearLogs();
  mock.manageChildren(1, [0,1], [1,2], [5,6], [0,3], [2]);
- expect(mock.commandLogs.length).toEqual(7);
- expect(mock.commandLogs.toString()).toEqual('DETACH+1+2,DETACH+1+1,DETACH+1+0,ATTACH+1+5+0,ATTACH+1+2+1,ATTACH+1+3+2,ATTACH+1+6+3');
+ expect(mock.commandLogs.length).toEqual(4);
+ expect(mock.commandLogs.toString()).toEqual('DETACH+1+2,MOVE+1+0,1+1,2,ATTACH+1+5+0,ATTACH+1+6+3');
  expect(mock.nativeElementMap.get(1).children.map((a) => a.tag).join(',')).toEqual('5,2,3,6');
  });