Semantics debugger shouldn't crash when reparenting nodes (#4782)

Our previous approach to detecting when we needed to remove semantics nodes
didn't account for reparenting.

packages/flutter/lib/src/widgets/semantics_debugger.dart

@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+import 'dart:collection';
 import 'dart:math' as math;
 
 import 'package:flutter/rendering.dart';
@@ -127,17 +128,54 @@ class _SemanticsDebuggerEntry {
            '${hasCheckedState ? isChecked ? "; checked" : "; unchecked" : ""}'
            ')';
   }
+
   String toStringDeep([ String prefix = '']) {
     if (prefix.length > 20)
       return '$prefix<ABORTED>\n';
     String result = '$prefix$this\n';
-    for (_SemanticsDebuggerEntry child in children.reversed) {
+    prefix += '  ';
-      prefix += '  ';
+    for (_SemanticsDebuggerEntry child in children) {
       result += '${child.toStringDeep(prefix)}';
     }
     return result;
   }
 
+  void updateWith(mojom.SemanticsNode node) {
+    if (node.flags != null) {
+      canBeTapped = node.flags.canBeTapped;
+      canBeLongPressed = node.flags.canBeLongPressed;
+      canBeScrolledHorizontally = node.flags.canBeScrolledHorizontally;
+      canBeScrolledVertically = node.flags.canBeScrolledVertically;
+      hasCheckedState = node.flags.hasCheckedState;
+      isChecked = node.flags.isChecked;
+    }
+    if (node.strings != null) {
+      assert(node.strings.label != null);
+      label = node.strings.label;
+    } else {
+      assert(label != null);
+    }
+    if (node.geometry != null) {
+      if (node.geometry.transform != null) {
+        assert(node.geometry.transform.length == 16);
+        // TODO(ianh): Replace this with a cleaner call once
+        //  https://github.com/google/vector_math.dart/issues/159
+        // is fixed.
+        List<double> array = node.geometry.transform;
+        transform = new Matrix4(
+          array[0],  array[1],  array[2],  array[3],
+          array[4],  array[5],  array[6],  array[7],
+          array[8],  array[9],  array[10], array[11],
+          array[12], array[13], array[14], array[15]
+        );
+      } else {
+        transform = null;
+      }
+      rect = new Rect.fromLTWH(node.geometry.left, node.geometry.top, node.geometry.width, node.geometry.height);
+    }
+    _updateMessage();
+  }
+
   int findDepth() {
     if (children == null || children.isEmpty)
       return 1;
@@ -153,7 +191,7 @@ class _SemanticsDebuggerEntry {
   );
 
   TextPainter textPainter;
-  void updateMessage() {
+  void _updateMessage() {
     List<String> annotations = <String>[];
     bool wantsTap = false;
     if (hasCheckedState) {
@@ -251,7 +289,7 @@ class _SemanticsDebuggerEntry {
   }
 }
 
-class _SemanticsDebuggerListener implements mojom.SemanticsListener {
+class _SemanticsDebuggerListener extends ChangeNotifier implements mojom.SemanticsListener {
   _SemanticsDebuggerListener._() {
     SemanticsNode.addListener(this);
   }
@@ -262,66 +300,40 @@ class _SemanticsDebuggerListener implements mojom.SemanticsListener {
     instance ??= new _SemanticsDebuggerListener._();
   }
 
-  Set<VoidCallback> _listeners = new Set<VoidCallback>();
+  _SemanticsDebuggerEntry get rootNode => _nodes[0];
-  void addListener(VoidCallback callback) {
+  final Map<int, _SemanticsDebuggerEntry> _nodes = <int, _SemanticsDebuggerEntry>{};
-    assert(!_listeners.contains(callback));
-    _listeners.add(callback);
-  }
-  void removeListener(VoidCallback callback) {
-    _listeners.remove(callback);
-  }
-
-  Map<int, _SemanticsDebuggerEntry> nodes = <int, _SemanticsDebuggerEntry>{};
 
   _SemanticsDebuggerEntry _updateNode(mojom.SemanticsNode node) {
-    _SemanticsDebuggerEntry entry = nodes.putIfAbsent(node.id, () => new _SemanticsDebuggerEntry(node.id));
+    final int id = node.id;
-    if (node.flags != null) {
+    _SemanticsDebuggerEntry entry = _nodes.putIfAbsent(id, () => new _SemanticsDebuggerEntry(id));
-      entry.canBeTapped = node.flags.canBeTapped;
+    entry.updateWith(node);
-      entry.canBeLongPressed = node.flags.canBeLongPressed;
-      entry.canBeScrolledHorizontally = node.flags.canBeScrolledHorizontally;
-      entry.canBeScrolledVertically = node.flags.canBeScrolledVertically;
-      entry.hasCheckedState = node.flags.hasCheckedState;
-      entry.isChecked = node.flags.isChecked;
-    }
-    if (node.strings != null) {
-      assert(node.strings.label != null);
-      entry.label = node.strings.label;
-    } else {
-      assert(entry.label != null);
-    }
-    if (node.geometry != null) {
-      if (node.geometry.transform != null) {
-        assert(node.geometry.transform.length == 16);
-        // TODO(ianh): Replace this with a cleaner call once
-        //  https://github.com/google/vector_math.dart/issues/159
-        // is fixed.
-        List<double> array = node.geometry.transform;
-        entry.transform = new Matrix4(
-          array[0],  array[1],  array[2],  array[3],
-          array[4],  array[5],  array[6],  array[7],
-          array[8],  array[9],  array[10], array[11],
-          array[12], array[13], array[14], array[15]
-        );
-      } else {
-        entry.transform = null;
-      }
-      entry.rect = new Rect.fromLTWH(node.geometry.left, node.geometry.top, node.geometry.width, node.geometry.height);
-    }
-    entry.updateMessage();
     if (node.children != null) {
-      Set<_SemanticsDebuggerEntry> oldChildren = new Set<_SemanticsDebuggerEntry>.from(entry.children ?? const <_SemanticsDebuggerEntry>[]);
+      if (entry.children != null)
-      entry.children?.clear();
+        entry.children.clear();
-      entry.children ??= new List<_SemanticsDebuggerEntry>();
+      else
+        entry.children = new List<_SemanticsDebuggerEntry>();
       for (mojom.SemanticsNode child in node.children)
         entry.children.add(_updateNode(child));
-      Set<_SemanticsDebuggerEntry> newChildren = new Set<_SemanticsDebuggerEntry>.from(entry.children);
-      Set<_SemanticsDebuggerEntry> removedChildren = oldChildren.difference(newChildren);
-      for (_SemanticsDebuggerEntry oldChild in removedChildren)
-        nodes.remove(oldChild.id);
     }
     return entry;
   }
 
+  void _removeDetachedNodes() {
+    // TODO(abarth): We should be able to keep this table updated without
+    // walking the entire tree.
+    Set<int> detachedNodes = new Set<int>.from(_nodes.keys);
+    Queue<_SemanticsDebuggerEntry> unvisited = new Queue<_SemanticsDebuggerEntry>();
+    unvisited.add(rootNode);
+    while (unvisited.isNotEmpty) {
+      _SemanticsDebuggerEntry node = unvisited.removeFirst();
+      detachedNodes.remove(node.id);
+      if (node.children != null)
+        unvisited.addAll(node.children);
+    }
+    for (int id in detachedNodes)
+      _nodes.remove(id);
+  }
+
   int generation = 0;
 
   @override
@@ -329,12 +341,12 @@ class _SemanticsDebuggerListener implements mojom.SemanticsListener {
     generation += 1;
     for (mojom.SemanticsNode node in nodes)
       _updateNode(node);
-    for (VoidCallback listener in _listeners)
+    _removeDetachedNodes();
-      listener();
+    notifyListeners();
   }
 
   _SemanticsDebuggerEntry _hitTest(Point position, _SemanticsDebuggerEntryFilter filter) {
-    return nodes[0]?.hitTest(position, filter);
+    return rootNode?.hitTest(position, filter);
   }
 
   void handleTap(Point position) {
@@ -370,10 +382,8 @@ class _SemanticsDebuggerPainter extends CustomPainter {
 
   @override
   void paint(Canvas canvas, Size size) {
-    _SemanticsDebuggerListener.instance.nodes[0]?.paint(
+    _SemanticsDebuggerEntry rootNode = _SemanticsDebuggerListener.instance.rootNode;
-      canvas,
+    rootNode?.paint(canvas, rootNode.findDepth());
-      _SemanticsDebuggerListener.instance.nodes[0].findDepth()
-    );
     if (pointerPosition != null) {
       Paint paint = new Paint();
       paint.color = const Color(0x7F0090FF);

packages/flutter/test/widget/semantics_6_test.dart

@@ -1,47 +0,0 @@
-// Copyright 2015 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-import 'package:flutter/material.dart';
-import 'package:flutter/widgets.dart';
-import 'package:flutter_test/flutter_test.dart';
-
-void main() {
-  testWidgets('Semantics 6 - SemanticsDebugger smoke test', (WidgetTester tester) async {
-
-    // This is a smoketest to verify that adding a debugger doesn't crash.
-
-    await tester.pumpWidget(
-      new Stack(
-        children: <Widget>[
-          new Semantics(),
-          new Semantics(
-            container: true
-          ),
-          new Semantics(
-            label: 'label'
-          ),
-        ]
-      )
-    );
-
-    await tester.pumpWidget(
-      new SemanticsDebugger(
-        child: new Stack(
-          children: <Widget>[
-            new Semantics(),
-            new Semantics(
-              container: true
-            ),
-            new Semantics(
-              label: 'label'
-            ),
-          ]
-        )
-      )
-    );
-
-    expect(true, isTrue); // expect that we reach here without crashing
-
-  });
-}

packages/flutter/test/widget/semantics_debugger_test.dart

@@ -0,0 +1,109 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+import 'package:flutter/material.dart';
+import 'package:flutter/widgets.dart';
+import 'package:flutter_test/flutter_test.dart';
+
+void main() {
+  testWidgets('SemanticsDebugger smoke test', (WidgetTester tester) async {
+    // This is a smoketest to verify that adding a debugger doesn't crash.
+    await tester.pumpWidget(
+      new Stack(
+        children: <Widget>[
+          new Semantics(),
+          new Semantics(
+            container: true
+          ),
+          new Semantics(
+            label: 'label'
+          ),
+        ]
+      )
+    );
+
+    await tester.pumpWidget(
+      new SemanticsDebugger(
+        child: new Stack(
+          children: <Widget>[
+            new Semantics(),
+            new Semantics(
+              container: true
+            ),
+            new Semantics(
+              label: 'label'
+            ),
+          ]
+        )
+      )
+    );
+
+    expect(true, isTrue); // expect that we reach here without crashing
+  });
+
+  testWidgets('SemanticsDebugger reparents subtree', (WidgetTester tester) async {
+    GlobalKey key = new GlobalKey();
+
+    await tester.pumpWidget(
+      new SemanticsDebugger(
+        child: new Stack(
+          children: <Widget>[
+            new Semantics(label: 'label1'),
+            new Positioned(
+              key: key, left: 0.0, top: 0.0, width: 100.0, height: 100.0,
+              child: new Semantics(label: 'label2')
+            ),
+          ]
+        )
+      )
+    );
+
+    await tester.pumpWidget(
+      new SemanticsDebugger(
+        child: new Stack(
+          children: <Widget>[
+            new Semantics(label: 'label1'),
+            new Semantics(
+              container: true,
+              child: new Stack(
+                children: <Widget>[
+                  new Positioned(
+                    key: key, left: 0.0, top: 0.0, width: 100.0, height: 100.0,
+                    child: new Semantics(label: 'label2')
+                  ),
+                  new Semantics(label: 'label3'),
+                ]
+              )
+            )
+          ]
+        )
+      )
+    );
+
+    await tester.pumpWidget(
+      new SemanticsDebugger(
+        child: new Stack(
+          children: <Widget>[
+            new Semantics(label: 'label1'),
+            new Semantics(
+              container: true,
+              child: new Stack(
+                children: <Widget>[
+                  new Positioned(
+                    key: key, left: 0.0, top: 0.0, width: 100.0, height: 100.0,
+                    child: new Semantics(label: 'label2')
+                  ),
+                  new Semantics(label: 'label3'),
+                  new Semantics(label: 'label4'),
+                ]
+              )
+            )
+          ]
+        )
+      )
+    );
+
+    expect(tester.takeException(), isNull);
+  });
+}