Make DecoratedBox repaint after being reparented (#13927)

Fixes https://github.com/flutter/flutter/issues/12553

The root cause of https://github.com/flutter/flutter/issues/12553 was that when the refresh indicator went away, which happened after the avatars had started their image loads but before the avatars had loaded, the DecoratedBoxes, along with the rest of the list, got GlobalKey-reparented, which caused the RenderDecoratedBox objects to unregister from the DecorationImagePainters, but they never re-registered since the whole subtree was in a RepaintBoundary and was therefore not explicitly repainted when the tree got reattached.

This fixes the bug by explicitly requiring any RenderDecoratedBox to repaint when it's reattached. This is probably a little more aggressive than required; we could probably expose a flag on Decoration that says whether or not the onChanged handler will ever be invoked, and only call markNeedsPaint if that's true, but we'll do that if it turns out that there's a performance issue here.

(This patch also introduces a bunch of improved debugging information that I used to track down the bug.)

packages/flutter/lib/src/painting/box_decoration.dart

@@ -421,4 +421,9 @@ class _BoxDecorationPainter extends BoxPainter {
       textDirection: configuration.textDirection,
     );
   }
+
+  @override
+  String toString() {
+    return 'BoxPainter for $_decoration';
+  }
 }

packages/flutter/lib/src/painting/decoration_image.dart

@@ -25,7 +25,7 @@ enum ImageRepeat {
   repeatY,
 
   /// Leave uncovered portions of the box transparent.
-  noRepeat
+  noRepeat,
 }
 
 /// An image for a box decoration.
@@ -187,7 +187,7 @@ class DecorationImage {
 /// This object should be disposed using the [dispose] method when it is no
 /// longer needed.
 class DecorationImagePainter {
-  DecorationImagePainter._(this._details, this._onChanged);
+  DecorationImagePainter._(this._details, this._onChanged) : assert(_details != null);
 
   final DecorationImage _details;
   final VoidCallback _onChanged;
@@ -210,9 +210,6 @@ class DecorationImagePainter {
   /// then the `onChanged` callback passed to [DecorationImage.createPainter]
   /// will be called.
   void paint(Canvas canvas, Rect rect, Path clipPath, ImageConfiguration configuration) {
-    if (_details == null)
-      return;
-
     assert(canvas != null);
     assert(rect != null);
     assert(configuration != null);
@@ -287,6 +284,11 @@ class DecorationImagePainter {
   void dispose() {
     _imageStream?.removeListener(_imageListener);
   }
+
+  @override
+  String toString() {
+    return '$runtimeType(stream: $_imageStream, image: $_image) for $_details';
+  }
 }
 
 /// Paints an image into the given rectangle on the canvas.

packages/flutter/lib/src/painting/image_stream.dart

@@ -155,9 +155,6 @@ class ImageStream extends Diagnosticable {
   /// happens.
   Object get key => _completer != null ? _completer : this;
 
-  @override
-  String toStringShort() => '$runtimeType';
-
   @override
   void debugFillProperties(DiagnosticPropertiesBuilder properties) {
     super.debugFillProperties(properties);
@@ -181,10 +178,10 @@ class ImageStream extends Diagnosticable {
 /// Base class for those that manage the loading of [dart:ui.Image] objects for
 /// [ImageStream]s.
 ///
-/// This class is rarely used directly. Generally, an [ImageProvider] subclass
-/// will return an [ImageStream] and automatically configure it with the right
-/// [ImageStreamCompleter] when possible.
-class ImageStreamCompleter extends Diagnosticable {
+/// [ImageStreamListener] objects are rarely constructed directly. Generally, an
+/// [ImageProvider] subclass will return an [ImageStream] and automatically
+/// configure it with the right [ImageStreamCompleter] when possible.
+abstract class ImageStreamCompleter extends Diagnosticable {
   final List<ImageListener> _listeners = <ImageListener>[];
   ImageInfo _current;
 
@@ -240,9 +237,6 @@ class ImageStreamCompleter extends Diagnosticable {
     ));
   }
 
-  @override
-  String toStringShort() => '$runtimeType';
-
   /// Accumulates a list of strings describing the object's state. Subclasses
   /// should override this to have their information included in [toString].
   @override

packages/flutter/lib/src/rendering/proxy_box.dart

@@ -1561,6 +1561,12 @@ class RenderDecoratedBox extends RenderProxyBox {
     _painter?.dispose();
     _painter = null;
     super.detach();
+    // Since we're disposing of our painter, we won't receive change
+    // notifications. We mark ourselves as needing paint so that we will
+    // resubscribe to change notifications. If we didn't do this, then, for
+    // example, animated GIFs would stop animating when a DecoratedBox gets
+    // moved around the tree due to GlobalKey reparenting.
+    markNeedsPaint();
   }
 
   @override

packages/flutter/test/widgets/box_decoration_test.dart

@@ -2,13 +2,94 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-import 'package:flutter_test/flutter_test.dart';
-import 'package:flutter/material.dart';
-import 'package:flutter/widgets.dart';
+import 'dart:async';
+import 'dart:typed_data';
+import 'dart:ui' as ui show Image;
 
+import 'package:flutter/foundation.dart';
+import 'package:flutter/material.dart';
+import 'package:flutter/painting.dart';
+import 'package:flutter/widgets.dart';
+import 'package:flutter_test/flutter_test.dart';
+
+import '../painting/image_data.dart';
 import '../rendering/mock_canvas.dart';
 
-void main() {
+class TestImageProvider extends ImageProvider<TestImageProvider> {
+  TestImageProvider(this.future);
+
+  final Future<Null> future;
+
+  static ui.Image image;
+
+  @override
+  Future<TestImageProvider> obtainKey(ImageConfiguration configuration) {
+    return new SynchronousFuture<TestImageProvider>(this);
+  }
+
+  @override
+  ImageStreamCompleter load(TestImageProvider key) {
+    return new OneFrameImageStreamCompleter(
+      future.then<ImageInfo>((Null value) => new ImageInfo(image: image))
+    );
+  }
+}
+
+Future<Null> main() async {
+  TestImageProvider.image = await decodeImageFromList(new Uint8List.fromList(kTransparentImage));
+
+  testWidgets('DecoratedBox handles loading images', (WidgetTester tester) async {
+    final GlobalKey key = new GlobalKey();
+    final Completer<Null> completer = new Completer<Null>();
+    await tester.pumpWidget(
+      new KeyedSubtree(
+        key: key,
+        child: new DecoratedBox(
+          decoration: new BoxDecoration(
+            image: new DecorationImage(
+              image: new TestImageProvider(completer.future),
+            ),
+          ),
+        ),
+      ),
+    );
+    expect(tester.binding.hasScheduledFrame, isFalse);
+    completer.complete();
+    await tester.idle();
+    expect(tester.binding.hasScheduledFrame, isTrue);
+    await tester.pump();
+    expect(tester.binding.hasScheduledFrame, isFalse);
+  });
+
+  testWidgets('Moving a DecoratedBox', (WidgetTester tester) async {
+    final Completer<Null> completer = new Completer<Null>();
+    final Widget subtree = new KeyedSubtree(
+      key: new GlobalKey(),
+      child: new RepaintBoundary(
+        child: new DecoratedBox(
+          decoration: new BoxDecoration(
+            image: new DecorationImage(
+              image: new TestImageProvider(completer.future),
+            ),
+          ),
+        ),
+      ),
+    );
+    await tester.pumpWidget(subtree);
+    await tester.idle();
+    expect(tester.binding.hasScheduledFrame, isFalse);
+    await tester.pumpWidget(new Container(child: subtree));
+    await tester.idle();
+    expect(tester.binding.hasScheduledFrame, isFalse);
+    completer.complete(); // schedules microtask, does not run it
+    expect(tester.binding.hasScheduledFrame, isFalse);
+    await tester.idle(); // runs microtask
+    expect(tester.binding.hasScheduledFrame, isTrue);
+    await tester.pump();
+    await tester.idle();
+    expect(tester.binding.hasScheduledFrame, isFalse);
+  });
+
   testWidgets('Circles can have uniform borders', (WidgetTester tester) async {
     await tester.pumpWidget(
       new Container(

packages/flutter/test/widgets/image_test.dart

@@ -295,12 +295,12 @@ void main() {
     final TestImageProvider imageProvider = new TestImageProvider();
     await tester.pumpWidget(new Image(image: imageProvider));
     final State<Image> image = tester.state/*State<Image>*/(find.byType(Image));
-    expect(image.toString(), equalsIgnoringHashCodes('_ImageState#00000(stream: ImageStream(OneFrameImageStreamCompleter, unresolved, 1 listener), pixels: null)'));
+    expect(image.toString(), equalsIgnoringHashCodes('_ImageState#00000(stream: ImageStream#00000(OneFrameImageStreamCompleter#00000, unresolved, 1 listener), pixels: null)'));
     imageProvider.complete();
     await tester.pump();
-    expect(image.toString(), equalsIgnoringHashCodes('_ImageState#00000(stream: ImageStream(OneFrameImageStreamCompleter, [100×100] @ 1.0x, 1 listener), pixels: [100×100] @ 1.0x)'));
+    expect(image.toString(), equalsIgnoringHashCodes('_ImageState#00000(stream: ImageStream#00000(OneFrameImageStreamCompleter#00000, [100×100] @ 1.0x, 1 listener), pixels: [100×100] @ 1.0x)'));
     await tester.pumpWidget(new Container());
-    expect(image.toString(), equalsIgnoringHashCodes('_ImageState#00000(lifecycle state: defunct, not mounted, stream: ImageStream(OneFrameImageStreamCompleter, [100×100] @ 1.0x, 0 listeners), pixels: [100×100] @ 1.0x)'));
+    expect(image.toString(), equalsIgnoringHashCodes('_ImageState#00000(lifecycle state: defunct, not mounted, stream: ImageStream#00000(OneFrameImageStreamCompleter#00000, [100×100] @ 1.0x, 0 listeners), pixels: [100×100] @ 1.0x)'));
   });
 
   testWidgets('Image.memory control test', (WidgetTester tester) async {