Implement system fonts system channel listener #38930

chunhtai · 2019-08-20T23:10:33Z

Description

This pr add a system channel listener to repaint RenderParagraph and RenderEditable upon receiving system fonts change notification. We will have engine side change coming soon

flutter/engine#12276

Related Issues

#38556

Tests

I added the following tests:

RenderParagraph relayout upon system fonts changes
RenderEditable relayout upon system fonts changes

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

goderbauer

What about other objects/widgets that use TextPainter to measure text (example: CupertinoDataPicker or Material's TimePicker)? Do we not have to notify them as well so they can rebuild?

goderbauer · 2019-08-26T19:26:00Z

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

@@ -148,6 +149,21 @@ mixin RendererBinding on BindingBase, ServicesBinding, SchedulerBinding, Gesture
    renderView.scheduleInitialFrame();
  }

+  Set<SystemFontsClient> _systemFontsObserver;
+
+  /// Handler function that is called when the operating system notifies the


Since this is about painting text, PaintingBinding is probably a better place for this.

goderbauer · 2019-08-26T19:28:17Z

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

@@ -148,6 +149,21 @@ mixin RendererBinding on BindingBase, ServicesBinding, SchedulerBinding, Gesture
    renderView.scheduleInitialFrame();
  }

+  Set<SystemFontsClient> _systemFontsObserver;


There probably needs to be a public way to register listeners. Not all listeners are going to be RenderObjects (e.g. all the the non-RenderObject that use TextPainter, which are for example Widgets using CustomPaint like the TimePicker).

goderbauer · 2019-08-26T19:30:46Z

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

+  @override
+  void attach(covariant Object owner) {
+    super.attach(owner);
+    RendererBinding.instance._systemFontsObserver.add(this);


Could we just pass in markNeedsLayout directly here? That way, interested RenderObjects would just have to implement the mixin and are good to go. (It also avoids an extra function call when this gets called).

goderbauer · 2019-08-26T19:34:13Z

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

@@ -556,6 +556,9 @@ mixin WidgetsBinding on BindingBase, SchedulerBinding, GestureBinding, RendererB
      case 'memoryPressure':
        handleMemoryPressure();
        break;
+      case 'fontsChange':
+        handleSystemFontsChange();


It is odd that the message bubbles up all the way to the widget layer and is then redirected back to the rendering layer. What if you've implemented your own Widget system based on Flutter's rendering layer? You still want the font thing to work.

goderbauer · 2019-08-26T19:34:45Z

packages/flutter/test/rendering/system_fonts_test.dart

+
+  @override
+  void bringIntoView(TextPosition position) { }
+}


nit: end with blank line.

chunhtai · 2019-08-27T23:02:23Z

Ready for re review @goderbauer

chunhtai · 2019-08-29T19:26:55Z

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

@@ -246,7 +246,7 @@ abstract class WidgetsBindingObserver {
 }

 /// The glue between the widgets layer and the Flutter engine.
-mixin WidgetsBinding on BindingBase, SchedulerBinding, GestureBinding, RendererBinding, SemanticsBinding {
+mixin WidgetsBinding on BindingBase, ServicesBinding, SchedulerBinding, GestureBinding, RendererBinding, SemanticsBinding {


I need to include this to get passed doc parsing.

goderbauer · 2019-09-03T20:03:46Z

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

+/// Client for listening to system fonts change.
+///
+/// Using this mixin will cause it automatically subscribe to system channel.
+mixin SystemFontsClient on RenderObject {


Maybe call this "RelayoutWhenSystemFontsChangeMixin" to better describe what it does?

goderbauer · 2019-09-03T20:08:54Z

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

@@ -415,3 +415,20 @@ class RenderingFlutterBinding extends BindingBase with GestureBinding, ServicesB
    renderView.child = root;
  }
 }
+
+/// Client for listening to system fonts change.


"Mixin for [RenderObject] that will call [markNeedsLayout] whenever the system fonts change".

Can you also add an explanation why/when a developer should mix this in?

goderbauer · 2019-09-03T20:09:48Z

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

+  @override
+  void attach(covariant Object owner) {
+    super.attach(owner);
+    PaintingBinding.instance?.systemFonts?.addListener(markNeedsLayout);


Why would instance or systemFonts be null? Shouldn't they always be initialized when you get to this place?

goderbauer · 2019-09-03T20:09:54Z

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

+  @override
+  void detach() {
+    super.detach();
+    PaintingBinding.instance?.systemFonts?.removeListener(markNeedsLayout);


goderbauer · 2019-09-03T20:10:23Z

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

+/// Client for listening to system fonts change.
+///
+/// Using this mixin will cause it automatically subscribe to system channel.
+mixin SystemFontsClient on RenderObject {


We should probably find a better location then rendering/binding.dart for this.

goderbauer · 2019-09-03T20:15:46Z

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

+  }
+}
+
+class _SystemFonts extends Listenable {


Maybe _SystemFontsListener

goderbauer · 2019-09-03T20:24:33Z

packages/flutter/lib/src/cupertino/date_picker.dart

@@ -100,6 +100,18 @@ class _DatePickerLayoutDelegate extends MultiChildLayoutDelegate {
  }
 }

+/// Retrieves cached widths of columns in the input state.
+@visibleForTesting
+Map<int, double> debugGetCupertinoDatePickerStateCache(State<CupertinoDatePicker> state) {


This is awkward code to add to the framework. Can we find another way to test this?

goderbauer · 2019-09-03T20:26:51Z

packages/flutter/lib/src/cupertino/date_picker.dart

+  }
+
+  void _handleSystemFontsChange () {
+    didChangeDependencies();


calling didChangeDependecies seems awkward (especially because that one calls super). Maybe factor out what you need from didChangeDependencies into a seperate method that we can call fro both places?

goderbauer · 2019-09-03T20:26:59Z

packages/flutter/lib/src/cupertino/date_picker.dart

+  }
+
+  void _handleSystemFontsChange() {
+    didChangeDependencies();


goderbauer · 2019-09-03T20:31:49Z

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

@@ -47,6 +49,11 @@ mixin ServicesBinding on BindingBase {
    return const _DefaultBinaryMessenger._();
  }

+  /// Handler for flutter/system message channel.


Handler called for messages received on the [SystemChannels.system] call.

Other bindings may override this to respond to incoming system message.

goderbauer · 2019-09-09T16:16:11Z

packages/flutter/lib/src/cupertino/date_picker.dart

+  }
+
+  void _handleSystemFontsChange () {
+    estimatedColumnWidths.clear();


Put this statement into the setState closure? (That's the state getting changed here, right?)

goderbauer · 2019-09-09T16:16:28Z

packages/flutter/lib/src/cupertino/date_picker.dart

+  void _handleSystemFontsChange () {
+    estimatedColumnWidths.clear();
+    setState(() {
+      // didChangeDependencies should update the width if necessary.


This comment seems outdated?

goderbauer · 2019-09-09T16:17:52Z

packages/flutter/lib/src/cupertino/date_picker.dart

+  }
+
+  void _handleSystemFontsChange() {
+    _refreshEstimatedColumnWidths();


Put this inside the setState since that's causing the state to change?

goderbauer · 2019-09-09T16:18:02Z

packages/flutter/lib/src/cupertino/date_picker.dart

+  void _handleSystemFontsChange() {
+    _refreshEstimatedColumnWidths();
+    setState(() {
+      // didChangeDependencies should update the width if necessary.


This comment seems outdated?

goderbauer · 2019-09-09T16:19:12Z

packages/flutter/lib/src/cupertino/date_picker.dart

+  }
+
+  void _handleSystemFontsChange () {
+    estimatedColumnWidths.clear();


Maybe add a comment why we have to clear this on system font changes (e.g. because it was determined by measuring the width of text which may have changed with the new fonts).

goderbauer · 2019-09-09T16:24:49Z

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

+/// system fonts change
+///
+/// System fonts can change when OS install or remove a font. Use this mixin if
+/// the [RenderObject] uses [TextPainter] or text related painter to correctly


What's a text related painter?

using ui.Paragraph directly without going through text painter. I should update the doc comment

goderbauer · 2019-09-09T16:25:19Z

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

@@ -13,6 +13,7 @@ import 'package:flutter/services.dart';

 import 'package:vector_math/vector_math_64.dart';

+import 'binding.dart';


Can this import be removed?

goderbauer · 2019-09-09T16:25:58Z

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

@@ -47,6 +49,13 @@ mixin ServicesBinding on BindingBase {
    return const _DefaultBinaryMessenger._();
  }

+  /// Handler called for messages received on the [SystemChannels.system] call.


call -> message channel.

goderbauer · 2019-09-09T16:26:07Z

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

@@ -47,6 +49,13 @@ mixin ServicesBinding on BindingBase {
    return const _DefaultBinaryMessenger._();
  }

+  /// Handler called for messages received on the [SystemChannels.system] call.
+  ///
+  /// Other bindings may override this to respond to incoming system message.


message -> messages.

goderbauer · 2019-09-09T16:27:01Z

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

-      case 'memoryPressure':
-        handleMemoryPressure();
-        break;
+    if (type == 'memoryPressure') {


Why the change from a switch?

googlebot · 2019-09-13T18:26:44Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-09-16T19:53:02Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

chunhtai · 2019-09-16T20:09:42Z

@goderbauer this is ready to review.

chunhtai · 2019-09-16T20:12:16Z

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

+  @override
+  void systemFontsDidChange() {
+    super.systemFontsDidChange();
+    _textPainter.markNeedsLayout();


I am not sure how do we test _textPainter has been marked needs layout. this also apply to all the other widget.
The current test only verify renderobject is dirty after font change. unless I added a bunch of visibleforTesting for all the widgets that have textpainter

goderbauer · 2019-09-16T22:44:26Z

packages/flutter/lib/src/cupertino/date_picker.dart

+  void _handleSystemFontsChange () {
+    setState(() {
+      // System fonts change might cause the text layout width to change.
+      // Clears cached widths to make sure we do not have updated values.


we do not have updated values? Isn't clearing the cache causing the values to get recalculated/updated?

Maybe:

"Clear cached width to ensure that they get recalculated with the new system fonts."

goderbauer · 2019-09-16T22:44:45Z

packages/flutter/lib/src/cupertino/date_picker.dart

+  void _handleSystemFontsChange() {
+    setState(() {
+      // System fonts change might cause the text layout width to change.
+      // Clears cached widths to make sure we do not have updated values.


This one is not really clearing a cache? It's proactively recalculating the widths?

(You could probably just remove the second line.)

goderbauer · 2019-09-16T22:48:29Z

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

@@ -79,6 +79,49 @@ mixin PaintingBinding on BindingBase, ServicesBinding {
    super.evict(asset);
    imageCache.clear();
  }
+
+  /// Listenable for OSes system fonts change event.


Suggested change

/// Listenable for OSes system fonts change event.

/// Listenable that notifies when the fonts available on the system have changed.

goderbauer · 2019-09-16T22:50:36Z

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

+    final String type = message['type'];
+    switch (type) {
+      case 'fontsChange':
+        final _SystemFontsNotifier notifier = systemFonts;


Instead of this slightly awkward cast you could have a correctly typed private _systemFonts variable and a public systemFonts getter for it.

goderbauer · 2019-09-16T22:51:49Z

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

@@ -161,6 +161,13 @@ class TextPainter {
  ui.Paragraph _paragraph;
  bool _needsLayout = true;

+  /// Marks this text painter's layout information as dirty and removes cached
+  /// information.


This should include information about when somebody may want to call this.

goderbauer · 2019-09-16T22:52:50Z

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

+/// text when it happens.
+mixin RelayoutWhenSystemFontsChangeMixin on RenderObject {
+
+  /// A callback that is called when system fonts has changed.


Called when system fonts have changed.

goderbauer · 2019-09-16T22:53:00Z

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

+
+  /// A callback that is called when system fonts has changed.
+  ///
+  /// Subclass should override this method to clear any extra cache.


Subclasses should override this method to clear any cached values that depend on font-related metrics.

goderbauer · 2019-09-16T22:54:42Z

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

+mixin RelayoutWhenSystemFontsChangeMixin on RenderObject {
+
+  /// A callback that is called when system fonts has changed.
+  ///


By default, [mark needsLayout] is called on the [RenderObject] implementing this mixin.

goderbauer · 2019-09-16T22:57:29Z

packages/flutter/lib/src/material/slider.dart

+  @override
+  void systemFontsDidChange() {
+    super.systemFontsDidChange();
+    _labelPainter.markNeedsLayout();


Will _updateLabelPainter not automatically cause this to happen?

goderbauer · 2019-09-16T22:58:21Z

packages/flutter/lib/src/material/range_slider.dart

+  @override
+  void systemFontsDidChange() {
+    super.systemFontsDidChange();
+    _startLabelPainter.markNeedsLayout();


Will _updateLabelPainters not cause this to happen?

no, _updateLabelPainters will set the same property of existing textpainter, which will all get shortcircuited.

goderbauer

LGTM

goderbauer · 2019-09-18T23:02:26Z

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

+
+  /// A callback that is called when system fonts have changed.
+  ///
+  /// By default, [mark needsLayout] is called on the [RenderObject]


nit: no space after mark. (sorry...)

* Implement system fonts system channel listener

fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Aug 20, 2019

googlebot added the cla: yes label Aug 20, 2019

chunhtai requested a review from goderbauer August 21, 2019 20:09

goderbauer reviewed Aug 26, 2019

View reviewed changes

chunhtai force-pushed the issues/38556 branch from 2b5d826 to e67a278 Compare August 27, 2019 20:38

chunhtai force-pushed the issues/38556 branch from dda3317 to b54967e Compare August 28, 2019 21:19

chunhtai commented Aug 29, 2019

View reviewed changes

goderbauer reviewed Sep 3, 2019

View reviewed changes

chunhtai force-pushed the issues/38556 branch from 14dc007 to e0856ae Compare September 6, 2019 20:39

goderbauer reviewed Sep 9, 2019

View reviewed changes

chunhtai force-pushed the issues/38556 branch from 3071459 to f8d5bb9 Compare September 9, 2019 17:20

chunhtai added the work in progress; do not review label Sep 11, 2019

googlebot added cla: no and removed cla: yes labels Sep 13, 2019

chunhtai mentioned this pull request Sep 13, 2019

Add system font change listener for windows flutter/engine#12276

Merged

chunhtai removed the work in progress; do not review label Sep 16, 2019

chunhtai force-pushed the issues/38556 branch from 1a86f0e to 2655235 Compare September 16, 2019 19:52

googlebot added cla: yes and removed cla: no labels Sep 16, 2019

chunhtai commented Sep 16, 2019

View reviewed changes

goderbauer reviewed Sep 16, 2019

View reviewed changes

goderbauer approved these changes Sep 18, 2019

View reviewed changes

chunhtai force-pushed the issues/38556 branch 4 times, most recently from 974dfa6 to 7b41a5f Compare September 23, 2019 18:27

chunhtai added 2 commits September 24, 2019 09:41

Implement system fonts system channel listener

c48a041

fix test

75e1926

chunhtai force-pushed the issues/38556 branch from 614fefd to 75e1926 Compare September 24, 2019 16:42

chunhtai merged commit 0ca5e71 into flutter:master Sep 24, 2019

Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Sep 30, 2019

Implement system fonts system channel listener (flutter#38930)

36da84f

* Implement system fonts system channel listener

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

		@@ -13,6 +13,7 @@ import 'package:flutter/services.dart';

		import 'package:vector_math/vector_math_64.dart';

		import 'binding.dart';

	/// Listenable for OSes system fonts change event.
	/// Listenable that notifies when the fonts available on the system have changed.

Implement system fonts system channel listener #38930

Implement system fonts system channel listener #38930

Uh oh!

Conversation

chunhtai commented Aug 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

goderbauer left a comment

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!

chunhtai commented Aug 27, 2019

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!

googlebot commented Sep 13, 2019

Uh oh!

googlebot commented Sep 16, 2019

Uh oh!

chunhtai commented Sep 16, 2019

Uh oh!

chunhtai Sep 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

chunhtai commented Aug 20, 2019 •

edited

Loading

chunhtai Sep 16, 2019 •

edited

Loading