introduce localized text geometry in MaterialLocalizations by yjbanov · Pull Request #11829 · flutter/flutter

yjbanov · 2017-08-29T22:06:23Z

What's implemented:

language categories
localized font sizes and font weights
Theme.of automatically localizes text properties in ThemeData based on the nearest available Localizations widget

What's not implemented:

language-specific text baseline (missing in the spec)
line height (lives in ParagraphStyle and not themeable in Flutter yet?)

Fixes #11771

/cc @Hixie @HansMuller

Hixie · 2017-08-29T22:34:57Z

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

nit: space (or newline, typically preferred) after { for named arguments (and matching whitespace on the } side)

Hixie · 2017-08-29T22:36:01Z

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

Why is this configurable? This class is the en-US localisation but it's also the interface that other MaterialLocalizations implementations will implement or extend, so it doesn't really make sense for this to be a field.

Only needed it for testing. But now I'll wait for Hans' PR to go in so I can move this into the DefaultMaterialLocalizations.

Merged with Hans' code. No changes necessary for typography.

Hixie · 2017-08-29T22:36:42Z

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

this should just be hard-coded to the englishLike text theme, since this is the en-US default localisation.

This will become abstract after Hans' change.

Hixie · 2017-08-29T22:37:07Z

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

these should be public since other packages that implement localisations will need to be able to refer to them.

Hixie · 2017-08-29T22:38:30Z

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

i believe all the dense ones should have an ideographic baseline.

Hixie · 2017-08-29T22:38:52Z

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

I don't think this table should exist. The actual localizations are where this information belongs.

Should this go in the upcoming DefaultMaterialLocalizations? @HansMuller, WDYT?

Moved under material/i18n/typography_localizations.dart.

I continue to this this table shouldn't exist.

Hixie · 2017-08-29T22:40:19Z

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

I would probably put the call to MaterialLocalizations.of(context) here too, so that all the inherited-widget logic is in one place.

Hixie · 2017-08-29T22:41:59Z

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

how does this differ from the superclass implementation? if it does, please add a comment saying what the difference is

Done. Also deduped the logic a bit by moving it into a top-level function.

Hixie · 2017-08-29T22:44:14Z

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

we should add a TODO about the font fallback issue we discussed

Hixie · 2017-08-29T22:46:49Z

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

Past experience has suggested that it's better if the call sites do the defaulting rather than the .of functions. It's ok to return null if there's no _LocalizationsScope in scope (though then you should document that that will happen), but we shouldn't default.

The problem with defaulting is that people don't know the value came from window, so they don't think to set up the onLocaleChanged handler. We saw this a lot with MediaQuery.of defaulting its padding to window.padding if it couldn't find a MediaQuery.

Rolled back this change. Turns out I don't need it.

Hixie · 2017-08-29T22:47:10Z

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

Please document that this returns null if there's no _LocalizationsScope in scope.

yjbanov

D'oh! I forgot to hit the "Submit review" button.

yjbanov · 2017-08-30T16:22:56Z

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

yjbanov · 2017-08-30T16:23:41Z

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

Only needed it for testing. But now I'll wait for Hans' PR to go in so I can move this into the DefaultMaterialLocalizations.

yjbanov · 2017-08-30T16:27:59Z

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

This will become abstract after Hans' change.

yjbanov · 2017-08-30T16:28:52Z

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

yjbanov · 2017-08-30T16:31:15Z

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

Should this go in the upcoming DefaultMaterialLocalizations? @HansMuller, WDYT?

yjbanov · 2017-08-30T16:51:00Z

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

yjbanov · 2017-08-30T17:03:40Z

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

Done. Also deduped the logic a bit by moving it into a top-level function.

yjbanov · 2017-08-30T17:06:05Z

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

yjbanov · 2017-08-30T17:08:00Z

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

Rolled back this change. Turns out I don't need it.

yjbanov · 2017-08-30T17:25:44Z

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

yjbanov · 2017-09-08T21:04:32Z

Merged this with Hans' changes. This is ready for review again. PTAL.

Hixie · 2017-09-09T19:54:44Z

I don't think we should have the table. The kind of typography needed should be a part of the actual localization object, just like whether we're LTR or RTL, what the strings are, etc. That way there's no difference between how we do things and how someone would create a new localization. It's a cleaner separation of concerns and layering.

yjbanov · 2017-09-12T15:52:27Z

I don't think we should have the table.

WDYT of fcb19a3?

Hixie · 2017-09-12T16:46:15Z

That's much better, but it seems like it'd be even better if the localizations just returned the TextTheme directly.

yjbanov · 2017-09-12T17:01:59Z

it'd be even better if the localizations just returned the TextTheme directly

Yep, it does. This PR adds MaterialLocalizations.localTextGeometry getter. The fact that everything is stuffed in a global const map is an implementation detail of our current i18n infrastructure, which we can improve without breaking changes later.

Hixie · 2017-09-12T17:48:53Z

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

Does "The text styles provided by this theme are inherited" mean that localTextGeometry's TextStyles will be inherit:true, or inherit:false? I think we should be more explicit about that.

Hixie · 2017-09-12T17:51:25Z

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

just remove @immutable on the superclass.

Hixie · 2017-09-12T17:52:44Z

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

are these fontSizes/fontWeights/baselines ever actually used? Can we get away with not specifying them?

Depends on what you mean by "actually used". Today we support using Typography without the Theme. Example:

testWidgets('geometry defaults are used', (WidgetTester tester) { final Typography typography = new Typography(platform: TargetPlatform.android); tester.pumpWidget(new Directionality( textDirection: TextDirection.ltr, child: new Text('Hello', style: typography.white.body1), )); });

I'm not sure if our sample apps use it in this way. Probably rarely or not at all.

Removed fontSizes/fontWeights/baselines in 98612e0. Wasn't a big deal for our tests.

yjbanov · 2017-09-15T00:43:08Z

PTAL

yjbanov · 2017-09-18T17:32:41Z

Ping? The PR keeps getting outdated as things get added to localizations.dart. I'd like to submit it asap.

Hixie · 2017-09-20T21:47:23Z

Hixie · 2017-09-25T19:13:48Z

This caused a noticeably regression of the stock_build_iteration benchmark.

yjbanov · 2017-09-25T19:49:15Z

Looking into it: #12247

tvolkert · 2017-09-29T01:20:16Z

FYI, this broke tests in package:flutter_markdown - MarkdownStyleSheet.fromTheme() assumes that it's passed a complete ThemeData instance.

Should:

MarkdownStyleSheet.fromTheme() assert that the theme data is complete (in order to provide a better failure mode than null * 0.85) and flutter_markdown_test.dart be updated to pass a complete theme data
MarkdownStyleSheet.fromTheme() be updated to be tolerant of incomplete theme data?

Hixie · 2017-09-29T19:26:13Z

MarkdownStyleSheet.fromTheme() should assert that the theme data is complete.

googlebot added the cla: yes label Aug 29, 2017

Hixie reviewed Aug 29, 2017

View reviewed changes

yjbanov commented Sep 6, 2017

View reviewed changes

yjbanov force-pushed the typography branch from 9db66d6 to b4775da Compare September 8, 2017 21:02

yjbanov force-pushed the typography branch from afbec69 to 6bb3d44 Compare September 12, 2017 16:07

Hixie reviewed Sep 12, 2017

View reviewed changes

yjbanov force-pushed the typography branch 2 times, most recently from 8da8191 to 98612e0 Compare September 15, 2017 00:40

yjbanov force-pushed the typography branch 2 times, most recently from ba87dc7 to 54fe7ee Compare September 21, 2017 00:19

yjbanov added 5 commits September 22, 2017 12:44

introduce localized text geometry in MaterialLocalizations

3dd43a4

remove geometry from color text themes

8d802e0

fix merge conflict

5cd9640

optional Localizations

cec33c5

fix fallback; test; docs

4fc27fa

yjbanov force-pushed the typography branch from 54fe7ee to 4fc27fa Compare September 22, 2017 19:50

yjbanov merged commit b9e1be9 into flutter:master Sep 22, 2017

yjbanov mentioned this pull request Sep 25, 2017

stock_build_iteration benchmark regressed with typography i18n #12247

Closed

DaveShuckerow mentioned this pull request Aug 7, 2018

Process: checking that all strings are translated before a release of the Flutter Gallery #20271

Closed

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

Conversation

yjbanov commented Aug 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

yjbanov 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!

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!

yjbanov commented Sep 8, 2017

Uh oh!

Hixie commented Sep 9, 2017

Uh oh!

yjbanov commented Sep 12, 2017

yjbanov commented Aug 29, 2017 •

edited

Loading