Implementing TextScaler for nonlinear text scaling #42062

LongCatIsLooong · 2023-05-16T00:18:01Z

platformTextScaler doesn't need to be per window, assuming the SP -> DiP mapping is always the same on the same device (unless the user changes the preference), and does not depend on the display device's pixel density (it's confirmed).

Design doc: https://docs.google.com/document/d/1-DlElw3zWRDlqoc9z4YfkU9PbBwRTnc7NhLM97ljGwk/edit#

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

…-scale

goderbauer · 2023-05-17T18:06:44Z

lib/ui/platform_dispatcher.dart

  _PlatformConfiguration _configuration = const _PlatformConfiguration();
+  // The expression is evaluated when `platformTextScaler` is accessed for the
+  // first time.
+  late final bool _platformTextScalerImplemented = _getScaledFontSize(14) != -1;


Why these magic numbers: "14" and "-1"?

14 is arbitrarily chosen. -1 is the error code for unimplemented. I'll add that to the comment.

even better, define a constant static const int _kUnimplementedErrorCode = -1

goderbauer · 2023-05-17T18:07:41Z

lib/ui/platform_dispatcher.dart

-  /// A callback that is invoked whenever [textScaleFactor] changes value.
+  /// Deprecated. Will be removed in a future version of Flutter.
+  ///
+  /// This field is renamed to [onTextScalerChanged].


nit: it is not just a rename, right? The semantics also changed, presumably, it fires when textScaler changes and not when textscalefactor changes?

Right now it is just a rename. It is invoked when the user changes the text scaling value. But in this PR we're not exposing textScaleFactor in public APIs so I thought the name "textScaleFactor" could be a bit confusing.

goderbauer · 2023-05-17T18:08:50Z

lib/ui/platform_dispatcher.dart

+  /// [onTextScaleFactorChanged] callback can be used to monitor such changes.
+  ///
+  /// Instead of directly using this getter, applications should typically use
+  /// `MediaQuery.scaledFontSizeOf` instead to retrive the scaled font size in a


put MediaQuery.scaledFontSizeOf in [] to link to it in docs?

that hasn't been added to the framework yet so the name is kinda fictional

should this only be mentioned when it is actually in the framework?

goderbauer · 2023-05-17T18:09:21Z

lib/ui/platform_dispatcher.dart

+  ///
+  ///  * [WidgetsBindingObserver], for a mechanism at the widgets layer to
+  ///    observe when this callback is invoked.
+  TextScaler get platformTextScaler {


Just to double check: This is on the PlatformDispatcher because it is a global thing for the entire app? This is not going to be a per-view setting?

hasn't been confirmed yet, I'll add a comment once it is.

It's been confirmed that this is per Android device not per monitor. I was also told device manufacturers will not be able to override the text scaling curve: maybe we can avoid using FFI/JNI and just hard-code the formula in dart.
For that we will need the Android build version and potentially some other information (it sounds like the curve is going to be different on TVs / watches than on phones). I think I'll ask for more information before proceeding since that sounds like a more performant and easier to implement solution than this.

If I add the TextScaler interface to the framework, what would be the easiest way to move it to dart:ui?

goderbauer · 2023-05-17T18:11:11Z

lib/ui/platform_dispatcher.dart

+  }
+
+  // Returns a negative number when there's an error:
+  // -1 : GetScaledFontSize is not implemented on the current platform.


Wondering if there's a way to make errors a little more structured to make this API a little more ergonomic and avoid magic numbers like -1.

This is an FFI call so using a primitive type is the easiest imo otherwise it would probably be a bit less efficient. The codomain can't go negative anyways.

What if -1 and -2 were constants in dart and constants on the other side of ffi. They would still be magic numbers over the wire but would be logically constants. If we documented that they needed to change together than I think that would be better. I find myself wishing for proto generated constants (comment above) and a LINT.IFCHANGED in google3.

If you do use constants do not forget to update the documentation on line platform_configuration.h link 230 and engine.h line 308.

Removed error code -2. Is there a way to share the error code definition between java and dart? The -1 literal is used once in each language so would comments that explain the return value suffice?

goderbauer · 2023-05-17T18:15:52Z

lib/ui/platform_dispatcher.dart

+        throw StateError('GetScaledFontSize is not implemented on the platform. This should not be reached.');
+      // Fallback to linear scaling when there's an error. Relevant error messages
+      // should already be printed to stderr.
+      case final double errorCode:


Why does this case exist?

currently the android implementation returns -2 if the delegate is not set, for example.

goderbauer · 2023-05-17T18:17:26Z

lib/web_ui/lib/platform_dispatcher.dart

+  /// Deprecated. Will be removed in a future version of Flutter.
+  ///
+  /// This field is renamed to [onTextScalerChanged].


I believe, doc comments do not need to be duplicated in the web_ui

goderbauer · 2023-05-17T18:17:50Z

lib/web_ui/lib/platform_dispatcher.dart

 }
+
+abstract class TextScaler {
+  /// Creates a TextScaler.


remove duplicated doc?

goderbauer · 2023-05-17T18:18:57Z

lib/web_ui/lib/platform_dispatcher.dart

+  }
+}
+
+final class _LinearTextScaler implements TextScaler {


Is there a way to share the implementation of this with regular dart:ui? This is just a straight-up copy, right? Would make this easier to maintain...

Same with _ClampedTextScaler.

(Makes me wonder if dart:ui should maybe just expose the _getScaledFontSize and the TestScaler stuff lives in the framework)...

Some stuff are still depending on text scale factor. If we move TextScaler to the framework then we'd have to expose textScaleFactor as part of the public API.

goderbauer · 2023-05-17T18:24:07Z

lib/ui/platform_dispatcher.dart

+}
+
+final class _SystemTextScaler extends TextScaler {
+  const _SystemTextScaler(this.textScaleFactor);


Why is the systemTextScaler dependent on the textScaleFactor?

chunhtai · 2023-06-05T22:22:36Z

lib/ui/platform_dispatcher.dart

  _PlatformConfiguration _configuration = const _PlatformConfiguration();
+  // The expression is evaluated when `platformTextScaler` is accessed for the
+  // first time.
+  late final bool _platformTextScalerImplemented = _getScaledFontSize(14) != -1;


even better, define a constant static const int _kUnimplementedErrorCode = -1

chunhtai · 2023-06-05T22:29:15Z

lib/ui/platform_dispatcher.dart

+  /// [onTextScaleFactorChanged] callback can be used to monitor such changes.
+  ///
+  /// Instead of directly using this getter, applications should typically use
+  /// `MediaQuery.scaledFontSizeOf` instead to retrive the scaled font size in a


should this only be mentioned when it is actually in the framework?

chunhtai · 2023-06-05T22:54:08Z

lib/ui/platform_dispatcher.dart

+
+  /// Returns a new [TextScaler] that restricts the scaled font size to within
+  /// the range `[minScaleFactor * fontSize, maxScaleFactor * fontSize]`.
+  TextScaler clamp(double minScaleFactor, double maxScaleFactor) {


Can you remind me again why we need this method? Also I imagine an app developer would probably more care about the clamping the font size after the scaling instead of clamping the scaling factor?

chunhtai · 2023-06-05T22:58:15Z

lib/ui/platform_dispatcher.dart

+      // should already be printed to stderr.
+      case final double errorCode:
+        assert(false, 'GetScaledFontSize failed with $errorCode');
+        return fontSize * textScaleFactor;


Is this a fallback behavior? shouldn't this be handled in embedding?

…-scale

…engine into android-sync-text-scale

goderbauer

LGTM

Leaving it to the Android embedder people (i.e. @reidbaker) to give approval if they're ok with the changes there.

goderbauer · 2023-08-16T20:43:28Z

lib/ui/platform_dispatcher.dart

+      return unscaledFontSize;
+    }
+
+    final int unscaledFloor = unscaledFontSize.floor();


I still think we should leave a comment in the code explaining this.

goderbauer · 2023-08-16T20:45:52Z

lib/ui/platform_dispatcher.dart

+      return cachedValue;
+    }
+
+    final double unscaledFontSizeInt = unscaledFontSize.toDouble();


nit: the variable has an Int suffix, but is typed double - shouldn't those two match?

goderbauer · 2023-08-16T20:50:11Z

lib/ui/platform_dispatcher.dart

+  // `unscaledFontSize`.
+  //
+  // Currently this is only implemented on newer versions of Android (SDK level
+  // 34, using the `TypedValue#applyDimension` API). This function returns -1 on


I am slightly confused about the error codes. Line 1366 made it seem as if -1 means you provided an invalid configuration ID, this documents that -1 means the platform doesn't support non-linear text scaling. Is it intentional that both use the same error code?

see also line 1394 below.

I think I forgot to update the comments. Updated now.

goderbauer · 2023-08-16T20:52:08Z

lib/ui/platform_dispatcher.dart

+  // (`_configuration.configurationId`). Using an incorrect id could result in
+  // an unrecoverable error.
+  //
+  // Returns -1 when the specified configurationId does not match any configuration.


document here that if the platform hasn't provided a configurationId int the _PlatformConfiguration it means this feature is not supported and _getScaledFontSize must not be called?

goderbauer · 2023-08-16T20:57:00Z

shell/common/engine.h

+    ///
+    /// @return     The scaled font size in logical pixels, or -1 when the given
+    ///             configuration_id did not match a valid configuration.
+    ///


May be worthwhile to call out here as well that this must only be called if you received a non-null configurationId from . And that otherwise the platform doesn't support this kind of scaling.

goderbauer · 2023-08-16T20:57:17Z

lib/ui/window/platform_configuration.h

+  ///
+  /// @return     The scaled font size in logical pixels, or -1 if the given
+  ///             configuration_id did not match a valid configuration.
+  ///


May be worthwhile to call out here as well that this must only be called if you received a non-null configurationId from . And that otherwise the platform doesn't support this kind of scaling.

goderbauer · 2023-08-16T20:57:47Z

shell/common/platform_view.h

+  ///
+  /// @return     The scaled font size in logical pixels, or -1 if the given
+  ///             configuration_id did not match a valid configuration.
+  ///


goderbauer · 2023-08-16T21:08:43Z

shell/platform/android/io/flutter/embedding/engine/systemchannels/SettingsChannel.java

  private static final String PLATFORM_BRIGHTNESS = "platformBrightness";
+  private static final String CONFIGURATION_ID = "configurationId";
+
+  // When hasNonlinearTextScalingSupport() returns false, this will not be initialized.


Is this because Java will only initialize static fields on first access and when hasNonlinearTextScalingSupport is false this is never accessed?

goderbauer · 2023-08-16T21:27:12Z

shell/platform/android/io/flutter/embedding/engine/systemchannels/SettingsChannel.java

+   * Flutter framework invokes a function to access the configuration with a generation identifier,
+   * this queue finds the configuration with that identifier and also cleans up configurations that
+   * are no longer needed.
+   */


Somewhere (maybe here?) can we leave a breadcrumb to the future Android API that could replace all this and make this simpler? Just as a reminder for future readers of this code.

…-scale

LongCatIsLooong · 2023-08-18T19:49:55Z

Hi @reidbaker could you take a look when you get a chance? I think I went through all comments but let me know if I missed any.

reidbaker · 2023-08-18T20:15:08Z

shell/platform/android/test/io/flutter/embedding/engine/systemchannels/SettingsChannelTest.java

+    verify(executor).send(eq("flutter/settings"), messageCaptor.capture(), isNull());
+  }
+
+  @Test


Should this test have a config = 34?

I'll give that a try. But iirc Robolectric does not support that api level yet and the test will fail.

it gave me

io.flutter.embedding.engine.systemchannels.SettingsChannelTest > configurationQueueWorks FAILED java.lang.IllegalArgumentException: API level 34 is not available at org.robolectric.plugins.UnknownSdk.verifySupportedSdk(UnknownSdk.java:45) at org.robolectric.RobolectricTestRunner.getSandbox(RobolectricTestRunner.java:298) at org.robolectric.RobolectricTestRunner.getSandbox(RobolectricTestRunner.java:66) at org.robolectric.internal.SandboxTestRunner$2.evaluate(SandboxTestRunner.java:244)

But you are calling an api 34 method. If roboletric does not support api 34 then update the dependency. Or at minimum use config minSdk to api 33 so that when api 34 is added this test starts running against api 34.

None of these methods directly or indirectly called is API 34 only I think.

If this code does not indirectly call 34 then shouldnt there be a test for the opposite of setDisplayMetricsDoesNothingOnAPILevel33. Something like setDisplayMetricsDoesXyzBehaviorOnApiLevel34?

robolectric/robolectric#8404, it seems there's no api 34 support yet.

edit: I was mistaken about roboletric api 34 support.

OK, can you file a follow up bug to add a test when we have a roboletric version that supports api 34?

LongCatIsLooong added 6 commits May 11, 2023 00:09

Sync MethodChannel

6064f58

fix test

16c7d02

Merge remote-tracking branch 'upstream/master' into android-sync-text…

edd2883

…-scale

fix test

88cb9a4

move TextScaler to dart:ui

6d76a0b

Merge remote-tracking branch 'upstream/master' into android-sync-text…

01c58db

…-scale

flutter-dashboard bot added platform-android platform-web Code specifically for the web engine labels May 16, 2023

LongCatIsLooong force-pushed the android-sync-text-scale branch from f76030e to d58bb16 Compare May 16, 2023 01:42

fix tests

387daf9

LongCatIsLooong force-pushed the android-sync-text-scale branch 6 times, most recently from eb43895 to 1ce8f1c Compare May 16, 2023 20:52

tests

37132f2

LongCatIsLooong force-pushed the android-sync-text-scale branch from 1ce8f1c to 37132f2 Compare May 16, 2023 21:28

clean up

3a8e001

LongCatIsLooong changed the title ~~WIP~~ Implementing TextScaler for nonlinear text scaling May 16, 2023

LongCatIsLooong requested review from chunhtai and goderbauer May 16, 2023 22:40

LongCatIsLooong marked this pull request as ready for review May 16, 2023 23:50

goderbauer reviewed May 17, 2023

View reviewed changes

LongCatIsLooong marked this pull request as draft May 18, 2023 17:48

chunhtai reviewed Jun 5, 2023

View reviewed changes

LongCatIsLooong added 4 commits July 17, 2023 13:19

Merge remote-tracking branch 'upstream/master' into android-sync-text…

a0b1346

…-scale

update

99c321a

fix race?

b399695

Add textScaleFactor to signature

aba732a

LongCatIsLooong added 4 commits August 7, 2023 21:10

review

49e5e4e

move tests

1226d22

Merge remote-tracking branch 'upstream/master' into android-sync-text…

ed49384

…-scale

Merge branch 'android-sync-text-scale' of github.com:LongCatIsLooong/…

e400c78

…engine into android-sync-text-scale

LongCatIsLooong requested review from chunhtai, goderbauer and reidbaker August 15, 2023 15:55

goderbauer reviewed Aug 16, 2023

View reviewed changes

LongCatIsLooong added 3 commits August 16, 2023 21:42

review

c249a35

Merge remote-tracking branch 'upstream/master' into android-sync-text…

fda4a3b

…-scale

fix test failure

9c4b731

reidbaker approved these changes Aug 18, 2023

View reviewed changes

LongCatIsLooong added 2 commits August 18, 2023 14:13

try adding @config(sdk = 34)

966d992

revert @config change

b63e678

LongCatIsLooong added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 18, 2023

auto-submit bot merged commit 5741f69 into flutter:main Aug 18, 2023

LongCatIsLooong deleted the android-sync-text-scale branch August 18, 2023 22:23

jonahwilliams mentioned this pull request Aug 20, 2023

Revert "Implementing TextScaler for nonlinear text scaling" #44882

Merged

engine-flutter-autoroll mentioned this pull request Aug 20, 2023

Roll Flutter Engine from f4bffdcf8536 to 2dcfb6cb7e49 (4 revisions) flutter/flutter#132914

Merged

Implementing TextScaler for nonlinear text scaling #42062

Implementing TextScaler for nonlinear text scaling #42062

Uh oh!

Conversation

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

Uh oh!

Pre-launch Checklist

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!

LongCatIsLooong May 18, 2023 • 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!

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!

LongCatIsLooong May 17, 2023 • 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!

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!

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

LongCatIsLooong commented May 16, 2023 •

edited

Loading

LongCatIsLooong May 18, 2023 •

edited

Loading

LongCatIsLooong May 17, 2023 •

edited

Loading