Initial implementation of AnnotatedRegion for system chrome by jonahwilliams · Pull Request #17672 · flutter/flutter

jonahwilliams · 2018-05-16T23:16:16Z

In order to fix the unpredictability of SystemChrome changes, I've added the AnnotatedRegion API as proposed by @Hixie on #4630.

Changes

Adds AnnotatedRegion<T>, a widget which places an AnnotatedRegionLayer<T> in the layer tree.
Adds Layer.findRegion(Offset, Type) which searches for the last AnnotatedRegionLayer that matches Type and contains Offset
- Checking that the offset is contained by the layer is not yet implemented.
Replaces direct calls to setSystemUiOverlayStyle with AnnotatedRegion<SystemUiOverlayStyle>
Calls setSystemUiOverlayStyle after the layer tree is built using findRegion.
Updates light and dark themes to always have dark app bars for Android - this is so enabling the default themes is non-breaking.

Questions

Can this change be submitted (which already massively improves SystemChrome behavior) before I update it with the proper hitTesting behavior?
Should we default to a dark theme if an AnnotatedRegion isn't found?
- Consider a case where our app has a light theme but no explicit AnnotatedRegion. We then push a route with an AnnotatedRegion and a dark theme. When we pop this route, currently the system ui theme remains dark.

Edit

Unfortunately I swapped the meaning of the brightness values for iOS in the engine, so I need to fix that separately

jonahwilliams · 2018-05-17T05:26:30Z

Getting closer, math is a bit off though.

Hixie · 2018-05-17T05:58:47Z

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


+  void _updateSystemChrome() {
+    final SystemUiOverlayStyle overlayStyle = layer.findRegion(
+      Offset.zero,


The suggestion I had was for when we just had one colour for everything. Now that we have two colours (at least), we probably want to independently get the details for the top and the bottom.

For the top, I'd probably aim for a point half way across the width, and half of the way down the top padding.
Similarly for the other sources.

Hixie · 2018-05-17T05:59:32Z

packages/flutter/lib/src/services/system_chrome.dart

-        _latestStyle = _pendingStyle;
-      }
-      _pendingStyle = null;
-    });


we might want to keep the old behaviour around in case anyone else wants to use this API, and doesn't have a way to guarantee they only call it once per frame.

Hixie · 2018-05-17T05:59:51Z

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

+}
+
+/// Render object for the [AnnotatedRegion].
+class AnnotatedRegionRenderObject<T> extends RenderProxyBox {


shouldn't this be in proxy_box?

The render_object, or the whole annotated_region API?

the render object.

Hixie · 2018-05-17T06:02:14Z

This looks fantastic.

before I update it with the proper hitTesting behavior?

not sure what you mean by "proper"? This seems pretty good already.

Should we default to a dark theme if an AnnotatedRegion isn't found?

We could have a default. I'd be tempted to just say if there's no information, we don't change it.

I would recommend adding more tests, e.g. to check if it works with crazy layer types like the leader/follower layers, if it works with scrolling of slivers, etc.

Hixie · 2018-05-17T06:03:50Z

When you fix whatever angered the bots, please see if you can also fix the _verifyNoBadImportsInFlutter logic to not crash when handling whatever you did... thanks :-)

Hixie · 2018-06-01T04:43:05Z

examples/hello_world/lib/main.dart

+import 'package:flutter/services.dart';
 import 'package:flutter/widgets.dart';

-void main() => runApp(const Center(child: const Text('Hello, world!', textDirection: TextDirection.ltr)));


Please don't change the hello world example. The whole point of this example is to be a one liner.

Sorry commited by mistake, was using this to debug values - will revert

Hixie · 2018-06-01T04:43:34Z

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


  @override
  Object findRegion(Offset offset, Type type) {
-    return super.findRegion(offset + this.offset, type);


Please name the argument something that doesn't shadow a member variable.

Hixie · 2018-06-01T04:44:16Z

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

  @override
  Object findRegion(Offset offset, Type type) {
-    return super.findRegion(offset + this.offset, type);
+    if (offset <= this.offset) {


Hixie · 2018-06-01T04:45:35Z

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

+        systemNavigationBarDividerColor: lowerOverlayStyle.systemNavigationBarDividerColor,
+      ));
+    } else if (upperOverlayStyle != null || lowerOverlayStyle != null) {
+      SystemChrome.setSystemUIOverlayStyle(upperOverlayStyle ?? lowerOverlayStyle);


It seems weird that when an upper overlay style goes out of view, suddenly the lower overlay style controls the top system ui overlay.

Hixie · 2018-06-18T06:50:06Z

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

+  S find<S>(Offset regionOffset) {
+    if (_invertedTransform == null) {
+      _invertedTransform ??= new Matrix4.zero();
+      _transform?.copyInto(_invertedTransform)?.invert();


there's a method or constructor or something that'll get you a new matrix that represents the inverse of another, which'll save you having to manually create the new one.

Hixie · 2018-06-18T06:51:16Z

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

+  @override
+  S find<S>(Offset regionOffset) {
+    final Offset transformed = regionOffset - offset;
+    return super.find<S>(transformed);


i don't mind which we do, but we should be consistent in the style between this one and LeaderLayer's.

Hixie · 2018-06-18T06:52:16Z

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

+    }
+    final Vector4 vector = new Vector4(regionOffset.dx, regionOffset.dy, 0.0, 1.0);
+    final Vector4 result = _invertedTransform.transform(vector);
+    return super.find<S>(new Offset(result[0] - linkedOffset.dx, result[1] - linkedOffset.dy));


this doesn't seem to handle the case of link.leader == null && !showWhenUnlinked. I assume in that case we should just return null?

Hixie · 2018-06-18T06:53:09Z

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

+/// a [Size] is provided to this layer, then find will check if the provided
+/// offset is within the bounds of the layer.
+class AnnotatedRegionLayer<T> extends ContainerLayer {
+  /// Creates a new annotated layer.


These docs don't match our style guide.

(Because they don't tell you anything you can't tell from the identifiers. Maybe mention the contract on size, e.g. that if null it won't clip, or something.)

maybe say value can't be null? and assert it

Hixie · 2018-06-18T06:54:20Z

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

+  /// The [size] is optionally used to clip the hit-testing of [find].
+  ///
+  /// If not provided, all offsets are considered to be contained within this
+  /// layer, unless a parent layer applies a clip.


a parent -> an ancestor

Hixie · 2018-06-18T06:57:00Z

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

+    super.debugFillProperties(properties);
+    properties.add(new DiagnosticsProperty<T>('value', value));
+    if (size != null)
+      properties.add(new DiagnosticsProperty<Size>('size', size));


you can always add this property, even if it's null. Just set defaultValue to null.

Hixie · 2018-06-18T06:57:41Z

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

+///   * [Layer.find], for an example of how this value is retrieved.
+///   * [AnnotatedRegionLayer], the layer this render object creates.
+class AnnotatedRegionRenderObject<T> extends RenderProxyBox {
+  /// A value which can be retrieved using [Layer.find].


The lack of a constructor is very atypical for RenderObject subclasses.

Hixie · 2018-06-18T06:57:57Z

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

+///   * [AnnotatedRegionLayer], the layer this render object creates.
+class AnnotatedRegionRenderObject<T> extends RenderProxyBox {
+  /// A value which can be retrieved using [Layer.find].
+  T value;


shouldn't changing value cause markNeedsPaint?

Hixie · 2018-06-18T06:58:01Z

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

+  T value;
+
+  /// Whether the render object will pass its [size] to the [AnnotatedRegionLayer].
+  bool sized;


similarly here, shouldn't mutating sized cause markNeedsPaint?

Hixie · 2018-06-18T06:58:48Z

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

+    if (child != null) {
+      final AnnotatedRegionLayer<T> layer = new AnnotatedRegionLayer<T>(value, size: sized ? size : null);
+      context.pushLayer(layer, super.paint, offset);
+    }


why do you only push the layer if there's a child?

Hixie · 2018-06-18T06:59:35Z

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

+  @override
+  void updateRenderObject(BuildContext context, AnnotatedRegionRenderObject<T> renderObject) {
+    renderObject.value = value;
+    renderObject.sized = sized;


prefer cascade operator; see other updateRenderObject implementations

…e layer correctly in appBar

jonahwilliams · 2018-06-19T17:50:41Z

Found issue with TestRecordingCanvas and with the location of the annotated region layer in app bar. Both fixes, should be all green

Hixie · 2018-06-19T17:50:57Z

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

+  /// object to clip the results of [Layer.findRegion].
+  ///
+  /// Neither [value] nor [sized] can be null.
+  AnnotatedRegionRenderObject(T value, bool sized)


btw the naming scheme is usually RenderFoo for the render object, and Foo for the widget.

Hixie · 2018-06-19T17:51:25Z

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

+    : assert(value != null),
+      assert(sized != null),
+      _value = value,
+      _sized = sized;


isn't that set by the element?

The render object layer can be used directly, it doesn't have to be used via widgets. When used directly, the first child is usually set in the constructor. Look at the other classes in this file. :-)

ahhh right, fixed

…y canvas update

jonahwilliams · 2018-06-19T21:13:12Z

yaaay it passed

jonahwilliams · 2018-06-22T01:26:44Z

Friendly ping. I've been informed this feature is a blocker for customer mulligan.

Hixie · 2018-06-22T04:07:36Z

Hixie · 2018-06-22T04:07:47Z

Thanks for doing this, this is awesome!

Jonah Williams added 2 commits May 16, 2018 15:29

initial pass on annotated region for system chrome

c601303

make sure child layers update first, test in gallery

531f2f0

jonahwilliams added the work in progress; do not review label May 16, 2018

googlebot added the cla: yes label May 16, 2018

Jonah Williams and others added 12 commits May 16, 2018 16:36

change android bottom brightness to dark for both themes

2fd5b89

merge and remove work around for app bar

9625cc8

add simple smoke test for AnnotatedRegion

06fcdef

make AnnotatedRegionRenderObject not private

27d414d

remove gallery app change and remove all use of position in findRegion

70e1f35

add licenses

4b2bcc8

fix brightness values that I messed up and doc comment

0e986c2

fix brightness values

83422fa

fix brightness values

5c4e187

fix brightness values

7b2916c

fix colors and add annotated widget to gallery

f8beb7c

initial work on transforming offsets, probably wrong

42740b1

Hixie reviewed May 17, 2018

View reviewed changes

remove some incorrect logic, add test

3393ef0

Hixie reviewed Jun 1, 2018

View reviewed changes

Jonah Williams added 3 commits June 1, 2018 15:27

merge

e23f63c

Merge branch 'chromeo' of github.com:jonahwilliams/flutter into chromeo

56f5431

undo accidental changes

4518f13

Hixie reviewed Jun 18, 2018

View reviewed changes

Jonah Williams added 3 commits June 18, 2018 14:21

address comments and implement system chrome

c36ff6f

ensure child layers are painted in TestRecordingPaintingContext, plac…

0b0a10b

…e layer correctly in appBar

Merge branch 'master' of github.com:flutter/flutter into chromeo

e1ab6e8

Hixie reviewed Jun 19, 2018

View reviewed changes

Jonah Williams added 2 commits June 19, 2018 10:57

rename to renderAnnotatedRegion

28e5106

pass child to RenderObject, change to named params, fix test broken b…

46038e1

…y canvas update

Merge branch 'master' of github.com:flutter/flutter into chromeo

fb6f25d

jonahwilliams merged commit f5f0551 into flutter:master Jun 22, 2018

jonahwilliams deleted the chromeo branch June 22, 2018 05:23

jonahwilliams mentioned this pull request Jun 25, 2018

We should derive the system chrome ui overlay style from a layer #4630

Closed

KevinTheGray mentioned this pull request Jun 26, 2018

Transforming ListView to x scale of 0 throws an exception #18847

Closed

cbracken mentioned this pull request Sep 17, 2018

iOS top task bar is white background and white icons when flutter appbar is white. #12173

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 10, 2021

Conversation

jonahwilliams commented May 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Edit

Uh oh!

jonahwilliams commented May 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented May 17, 2018

Uh oh!

Hixie commented May 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented May 16, 2018 •

edited

Loading

Hixie Jun 18, 2018 •

edited

Loading