text: Bundle 'Source Code Pro' font in the app; use it for code blocks/spans in messages

[chrisbobbe]

Fixes: #64
Related: #65

[chrisbobbe]

Tested on iOS and Android, and the text appears in the new font and with an appropriate-looking weight.

[chrisbobbe]

Just updated with a newer version of the font. As @andersk points out:

Google Fonts provides old versions of Source Code Pro (upright 2.030, italic 1.050), compared to the current versions from Adobe (upright 2.042, italic 1.062).

I also updated this to add an entry for this font's license in LicenseRegistry. For a proposed UI to view the list of licenses tracked by LicenseRegistry, see:

• accounts ui: Offer Flutter's [LicensePage], to acknowledge software used by the app #101

[gnprice]

Thanks for taking care of this! And thanks @andersk for spotting those opportunities to use better versions of the fonts.

This approach looks good; specific comments below.

[gnprice]

There's only 9 of these possible values, in a fixed list of them. So it'd be simpler to just write out some comparisons explicitly:

if (wght < 450) {
  if (wght < 250) {
    if (wght < 150) return FontWeight.w100;
    else            return FontWeight.w200;
  } else {
    // …
  }
} else {
  // …
}

[chrisbobbe]

I was thinking about being agnostic to what the list was (except that it's non-empty and sorted), in case it changes in a new Flutter version. But I guess it's not super likely to change, and also we can have a test or an assert to alert us if it changes.

[gnprice]

From the name of this parameter and the dartdoc above, I'm not sure what it means.

Ah, I see — it's the weight to use if the platform requests bold. Adding "if" to the name would help a lot, I think, and not really be materially longer than it is already.

[gnprice]

Seems like it'd be a bug for a caller to specify one of these but not the other. Right?

In that case, would be good to specify that and have an assertion: assert((wght != null) == (wghtIfPlatformRequestsBold) != null));.

[gnprice]

Hmm, the TextStyle for this span had been a const, but now there's a couple of merge calls going on, plus everything that's in weightVariableTextStyle. That feels like more computation than I want to be repeating every time we build an inline code span — there could be a lot of those on the screen.

The immediate solution for that problem is to move this to a final static, i.e. a final variable at the top level of the file (just like _kCodeStyle and errorCodeStyle are after these changes). That way it'll be computed lazily, once.

That won't support passing it a BuildContext so it can check MediaQueryData.boldText. I think the way I'd like to handle that is to have the result of this whole computation (the TextStyle we get at the end of it) to live on some InheritedWidget, so that at this spot we can just efficiently look it up. Then that InheritedWidget can live at the MessageList (or possibly higher in the tree) so that the computation doesn't get repeated frequently.

[gnprice]

OK, I guess [Text.build] routinely does a [TextStyle.merge], to combine any style you pass it with the default text style. So that makes me feel a bit better that maybe that's cheap enough this is fine.

Still — these are spans, and there can potentially be a lot of those peppered through a paragraph, more densely than any normal UI is likely to have Text widgets. And we want users to be able to scroll through tons of messages quickly and have that be nice and smooth, which puts a level of strain on fast widget building that isn't typical for most UIs. Plus there's some more stuff going on inside that weightVariableTextStyle. So I think I'd like to stay paranoid about the performance here, and memoize this as a final static for now, with memoizing on an InheritedWidget as the plan for how to satisfy that TODO(a11y).

[gnprice]

This seems like a sign the parameters should just have type double in the first place.

[chrisbobbe]

From that page linked on the 1..1000 assert above—https://fonts.google.com/variablefonts#axis-definitions —it looks like the values on some axes are expected to "step" by less than one, making double necessary for those, but for wght, values are expected to step by one. That was my thinking behind using int, but double wouldn't be wrong, I suppose; it'd just let us pass around non-integer values that would get rounded, or floored, etc.

[gnprice]

I see, yeah.

Given that the underlying API takes double, though, I think it makes sense to have this take double, in order to remain as thin a layer as it can while doing its job.

[gnprice]

nit:

Suggested change

	Future<void> runCheck(
	String description,
	{
	required TextStyle Function(BuildContext context) styleBuilder,
	Future<void> runCheck(
	String description, {
	required TextStyle Function(BuildContext context) styleBuilder,

For some examples in the Flutter repo, see the Text constructors.

[gnprice]

corresponding nit:

Suggested change

	}
	) async {
	}) async {

[gnprice]

nit: the name says "run", but this function's actual behavior is to define a test (by calling testWidgets), not to run anything. (And if it did run anything, that'd be bad because it's not inside a test or testWidgets callback; whatever it does will happen unconditionally even when try to run only specific tests with e.g. flutter test --name.)

Let's give it a name starting with "test", following the pattern set by testWidgets itself. Perhaps testWeights?

[gnprice]

nit: indentation

[gnprice]

I think these would be cleaner with a little extension, outside the test code itself. It's only a tiny bit longer that way in total, and it keeps the noise out of this test code (making the logic easier to see) while keeping the boilerplate concentrated to be maximally repetitive and boring (which is good for spotting errors in it, compared with having it interspersed with non-repetitive other code as here.)

In particular, the isTrue here blends in pretty invisibly, whereas a line ..inherit.isTrue() would be quite clear.

It'd be fine for such an extension to live in the individual test file (down at the bottom), when not expecting to reuse it. For this one, perhaps better yet would be to inaugurate a test/flutter_checks.dart, similar to the stdlib_checks.dart I added in #112.

[chrisbobbe]

Thanks for the review! Revision pushed.

[chrisbobbe]

(Rebased to resolve conflicts with #115.)

[gnprice]

Thanks! All looks good, modulo small style comments below. Let's merge this after #101.

[gnprice]

nit: better to make this consistent with the similar lines around it:

Suggested change

	Subject<List<FontVariation>?> get fontVariations =>
	has((t) => t.fontVariations, 'fontVariations');
	Subject<List<FontVariation>?> get fontVariations => has((t) => t.fontVariations, 'fontVariations');

It makes the line somewhat long, but that's OK; the code is super boring, so the reader should just be skimming anyway to confirm the boringness. And keeping the format as boring as the code helps keep it easy to skim.

See also the Flutter style guide's discussion of this:
https://github.com./flutter/flutter/wiki/Style-guide-for-Flutter-repo#prefer-a-maximum-line-length-of-80-characters

Aim for a maximum line length of roughly 80 characters, but prefer going over if breaking the line would make it less readable, or if it would make the line less consistent with other nearby lines.

[gnprice]

nit: always a trailing comma before newline:

Suggested change

	required FontWeight expectedFontWeight
	required FontWeight expectedFontWeight,

See also:
https://github.com./flutter/flutter/wiki/Style-guide-for-Flutter-repo#use-a-trailing-comma-for-arguments-parameters-and-list-items-but-only-if-they-each-have-their-own-line

[gnprice]

Cleanest to give this its own test call with its own name. E.g.:

Suggested change

	test('clampVariableFontWeight', () {
	// Implementation assumes specific FontWeight values;
	// adapt if these change in a new Flutter version.
	check(FontWeight.values).deepEquals([
	FontWeight.w100, FontWeight.w200, FontWeight.w300,
	FontWeight.w400, FontWeight.w500, FontWeight.w600,
	FontWeight.w700, FontWeight.w800, FontWeight.w900]);
	test('clampVariableFontWeight: FontWeight has the assumed list of values', () {
	// Implementation assumes specific FontWeight values;
	// adapt if these change in a new Flutter version.
	check(FontWeight.values).deepEquals([
	FontWeight.w100, FontWeight.w200, FontWeight.w300,
	FontWeight.w400, FontWeight.w500, FontWeight.w600,
	FontWeight.w700, FontWeight.w800, FontWeight.w900,
	]);
	});
	test('clampVariableFontWeight', () {

[chrisbobbe]

Thanks for the review! Revision pushed.

Let's merge this after #101.

Sure, SGTM.

[gnprice]

Thanks! Looks good; merging.