From 7b706d5f63207aaf82d12a4b26233bc476771b1e Mon Sep 17 00:00:00 2001 From: Luan Nico Date: Thu, 18 Apr 2024 09:06:29 -0400 Subject: [PATCH] fix: Fix cascading and fallback propagation of text styles (#3129) Fix cascading and fallback propagation of text styles. While using text nodes and flame_markdown, I noticed that basic propagating of styles was not working. For example, if you create a standard document, it will have some margins by default. Even when setting it to zero, it was still overwritten by the default style. I noticed that there was no consistency in the methods `copyWith` and `merge` of which parameter/receiver should be the master and which should be the fallback. Both the implementations and the callers would use it either way indiscriminately. This consolidates the definition and adds comments to the methods enforcing the priority order. Also fixes the wrong precedence on `copyWith` methods. In order to fully test this, I had to expose a few internal details of text elements; I think this is fine since Dart supports the `@visibleForTesting` annotation, and it makes the tests cleaner. --- .../src/text/elements/group_text_element.dart | 4 + .../elements/text_painter_text_element.dart | 4 + .../lib/src/text/styles/document_style.dart | 42 ++--- .../lib/src/text/styles/flame_text_style.dart | 4 + .../src/text/styles/inline_text_style.dart | 14 +- packages/flame/test/text/text_style_test.dart | 146 ++++++++++++++++++ 6 files changed, 189 insertions(+), 25 deletions(-) create mode 100644 packages/flame/test/text/text_style_test.dart diff --git a/packages/flame/lib/src/text/elements/group_text_element.dart b/packages/flame/lib/src/text/elements/group_text_element.dart index a7e09fe7582..838b8fa3f9f 100644 --- a/packages/flame/lib/src/text/elements/group_text_element.dart +++ b/packages/flame/lib/src/text/elements/group_text_element.dart @@ -2,6 +2,7 @@ import 'dart:math'; import 'dart:ui'; import 'package:flame/text.dart'; +import 'package:meta/meta.dart'; class GroupTextElement extends InlineTextElement { GroupTextElement(List children) @@ -15,6 +16,9 @@ class GroupTextElement extends InlineTextElement { @override LineMetrics get metrics => _metrics; + @visibleForTesting + List get children => List.unmodifiable(_children); + @override void draw(Canvas canvas) { for (final child in _children) { diff --git a/packages/flame/lib/src/text/elements/text_painter_text_element.dart b/packages/flame/lib/src/text/elements/text_painter_text_element.dart index 3fefbd60d7f..30f1ff7c777 100644 --- a/packages/flame/lib/src/text/elements/text_painter_text_element.dart +++ b/packages/flame/lib/src/text/elements/text_painter_text_element.dart @@ -2,6 +2,7 @@ import 'dart:ui'; import 'package:flame/text.dart'; import 'package:flutter/rendering.dart' as flutter; +import 'package:meta/meta.dart'; class TextPainterTextElement extends InlineTextElement { TextPainterTextElement(this._textPainter) @@ -16,6 +17,9 @@ class TextPainterTextElement extends InlineTextElement { final flutter.TextPainter _textPainter; final LineMetrics _box; + @visibleForTesting + flutter.TextPainter get textPainter => _textPainter; + @override LineMetrics get metrics => _box; diff --git a/packages/flame/lib/src/text/styles/document_style.dart b/packages/flame/lib/src/text/styles/document_style.dart index cbaf6bcfe6a..786cd60b582 100644 --- a/packages/flame/lib/src/text/styles/document_style.dart +++ b/packages/flame/lib/src/text/styles/document_style.dart @@ -27,18 +27,18 @@ class DocumentStyle extends FlameTextStyle { BlockStyle? header4, BlockStyle? header5, BlockStyle? header6, - }) : _text = FlameTextStyle.merge(text, DocumentStyle.defaultTextStyle), - _boldText = FlameTextStyle.merge(boldText, BoldTextNode.defaultStyle), + }) : _text = FlameTextStyle.merge(DocumentStyle.defaultTextStyle, text), + _boldText = FlameTextStyle.merge(BoldTextNode.defaultStyle, boldText), _italicText = - FlameTextStyle.merge(italicText, ItalicTextNode.defaultStyle), + FlameTextStyle.merge(ItalicTextNode.defaultStyle, italicText), _paragraph = - FlameTextStyle.merge(paragraph, ParagraphNode.defaultStyle), - _header1 = FlameTextStyle.merge(header1, HeaderNode.defaultStyleH1), - _header2 = FlameTextStyle.merge(header2, HeaderNode.defaultStyleH2), - _header3 = FlameTextStyle.merge(header3, HeaderNode.defaultStyleH3), - _header4 = FlameTextStyle.merge(header4, HeaderNode.defaultStyleH4), - _header5 = FlameTextStyle.merge(header5, HeaderNode.defaultStyleH5), - _header6 = FlameTextStyle.merge(header6, HeaderNode.defaultStyleH6); + FlameTextStyle.merge(ParagraphNode.defaultStyle, paragraph), + _header1 = FlameTextStyle.merge(HeaderNode.defaultStyleH1, header1), + _header2 = FlameTextStyle.merge(HeaderNode.defaultStyleH2, header2), + _header3 = FlameTextStyle.merge(HeaderNode.defaultStyleH3, header3), + _header4 = FlameTextStyle.merge(HeaderNode.defaultStyleH4, header4), + _header5 = FlameTextStyle.merge(HeaderNode.defaultStyleH5, header5), + _header6 = FlameTextStyle.merge(HeaderNode.defaultStyleH6, header6); final InlineTextStyle? _text; final InlineTextStyle? _boldText; @@ -114,19 +114,25 @@ class DocumentStyle extends FlameTextStyle { width: other.width ?? width, height: other.height ?? height, padding: other.padding, - background: merge(other.background, background)! as BackgroundStyle, - paragraph: merge(other.paragraph, paragraph)! as BlockStyle, - header1: merge(other.header1, header1)! as BlockStyle, - header2: merge(other.header2, header2)! as BlockStyle, - header3: merge(other.header3, header3)! as BlockStyle, - header4: merge(other.header4, header4)! as BlockStyle, - header5: merge(other.header5, header5)! as BlockStyle, - header6: merge(other.header6, header6)! as BlockStyle, + text: FlameTextStyle.merge(_text, other.text), + boldText: FlameTextStyle.merge(_boldText, other.boldText), + italicText: FlameTextStyle.merge(_italicText, other.italicText), + background: merge(background, other.background) as BackgroundStyle?, + paragraph: merge(paragraph, other.paragraph) as BlockStyle?, + header1: merge(header1, other.header1) as BlockStyle?, + header2: merge(header2, other.header2) as BlockStyle?, + header3: merge(header3, other.header3) as BlockStyle?, + header4: merge(header4, other.header4) as BlockStyle?, + header5: merge(header5, other.header5) as BlockStyle?, + header6: merge(header6, other.header6) as BlockStyle?, ); } final Map> _mergedStylesCache = {}; + + /// Merges two [FlameTextStyle]s together, preferring the properties of + /// [style2] if present, falling back to the properties of [style1]. FlameTextStyle? merge(FlameTextStyle? style1, FlameTextStyle? style2) { if (style1 == null) { return style2; diff --git a/packages/flame/lib/src/text/styles/flame_text_style.dart b/packages/flame/lib/src/text/styles/flame_text_style.dart index 91856fe6052..465edac96b9 100644 --- a/packages/flame/lib/src/text/styles/flame_text_style.dart +++ b/packages/flame/lib/src/text/styles/flame_text_style.dart @@ -15,8 +15,12 @@ import 'package:flame/text.dart'; abstract class FlameTextStyle { const FlameTextStyle(); + /// Creates a new [FlameTextStyle], preferring the properties of [other] + /// if present, falling back to the properties of `this`. FlameTextStyle copyWith(covariant FlameTextStyle other); + /// Merges two [FlameTextStyle]s together, preferring the properties of + /// [style2] if present, falling back to the properties of [style1]. static T? merge(T? style1, T? style2) { if (style1 == null) { return style2; diff --git a/packages/flame/lib/src/text/styles/inline_text_style.dart b/packages/flame/lib/src/text/styles/inline_text_style.dart index dc2a00deb58..c3928291800 100644 --- a/packages/flame/lib/src/text/styles/inline_text_style.dart +++ b/packages/flame/lib/src/text/styles/inline_text_style.dart @@ -27,13 +27,13 @@ class InlineTextStyle extends FlameTextStyle { @override InlineTextStyle copyWith(InlineTextStyle other) { return InlineTextStyle( - color: color ?? other.color, - fontFamily: fontFamily ?? other.fontFamily, - fontSize: fontSize ?? other.fontSize, - fontScale: fontScale ?? other.fontScale, - fontWeight: fontWeight ?? other.fontWeight, - fontStyle: fontStyle ?? other.fontStyle, - letterSpacing: letterSpacing ?? other.letterSpacing, + color: other.color ?? color, + fontFamily: other.fontFamily ?? fontFamily, + fontSize: other.fontSize ?? fontSize, + fontScale: other.fontScale ?? fontScale, + fontWeight: other.fontWeight ?? fontWeight, + fontStyle: other.fontStyle ?? fontStyle, + letterSpacing: other.letterSpacing ?? letterSpacing, ); } diff --git a/packages/flame/test/text/text_style_test.dart b/packages/flame/test/text/text_style_test.dart new file mode 100644 index 00000000000..8e90c10dbee --- /dev/null +++ b/packages/flame/test/text/text_style_test.dart @@ -0,0 +1,146 @@ +import 'package:flame/src/text/elements/group_element.dart'; +import 'package:flame/src/text/elements/group_text_element.dart'; +import 'package:flame/text.dart'; +import 'package:flutter/rendering.dart'; +import 'package:test/test.dart'; + +void main() { + group('text styles', () { + test('document style defaults are applied', () { + final style = DocumentStyle(); + expect(style.text.fontSize, 16); + expect(style.boldText.fontWeight, FontWeight.bold); + expect(style.italicText.fontStyle, FontStyle.italic); + expect(style.paragraph.margin, const EdgeInsets.all(6)); + expect(style.paragraph.padding, EdgeInsets.zero); + }); + + test('document style parameters are respected', () { + final style = DocumentStyle( + text: InlineTextStyle( + fontSize: 8, + ), + boldText: InlineTextStyle( + fontWeight: FontWeight.w900, + ), + italicText: InlineTextStyle( + fontStyle: FontStyle.normal, + ), + paragraph: const BlockStyle( + margin: EdgeInsets.zero, + padding: EdgeInsets.zero, + ), + ); + expect(style.text.fontSize, 8); + expect(style.boldText.fontWeight, FontWeight.w900); + expect(style.italicText.fontStyle, FontStyle.normal); + expect(style.paragraph.margin, EdgeInsets.zero); + expect(style.paragraph.padding, EdgeInsets.zero); + }); + + test('document style can be copied', () { + final style = DocumentStyle( + text: InlineTextStyle( + fontSize: 8, + ), + boldText: InlineTextStyle( + fontWeight: FontWeight.w900, + ), + paragraph: const BlockStyle( + margin: EdgeInsets.zero, + padding: EdgeInsets.zero, + ), + background: BackgroundStyle( + color: const Color(0xFFFF00FF), + ), + ); + final newStyle = style.copyWith( + DocumentStyle( + text: InlineTextStyle( + fontSize: 10, + ), + boldText: InlineTextStyle( + fontWeight: FontWeight.w900, + ), + italicText: InlineTextStyle( + fontStyle: FontStyle.italic, + ), + paragraph: const BlockStyle( + margin: EdgeInsets.all(10), + padding: EdgeInsets.zero, + ), + ), + ); + expect(newStyle.text.fontSize, 10); + expect(newStyle.boldText.fontWeight, FontWeight.w900); + expect(newStyle.italicText.fontStyle, FontStyle.italic); + expect(newStyle.paragraph.margin, const EdgeInsets.all(10)); + expect(newStyle.paragraph.padding, EdgeInsets.zero); + expect( + newStyle.background!.backgroundPaint!.color, + const Color(0xFFFF00FF), + ); + }); + + test('styles are cascading', () { + final style = DocumentStyle( + width: 600.0, + height: 400.0, + text: InlineTextStyle( + fontFamily: 'Helvetica', + fontSize: 8, + ), + boldText: InlineTextStyle( + fontFamily: 'Arial', + fontSize: 10, + fontWeight: FontWeight.w900, + ), + italicText: InlineTextStyle( + fontFamily: 'Arial', + fontSize: 6, + fontStyle: FontStyle.italic, + ), + paragraph: BlockStyle( + text: InlineTextStyle( + fontSize: 12, + ), + ), + ); + final document = DocumentRoot([ + ParagraphNode.group([ + PlainTextNode('This is '), + BoldTextNode.simple('my'), + PlainTextNode(' town - '), + ItalicTextNode.simple('The Sheriff'), + ]), + ]); + + final element = document.format(style); + final groupElement = element.children.first as GroupElement; + final groupTextElement = groupElement.children.first as GroupTextElement; + final styles = groupTextElement.children.map((e) { + return (e as TextPainterTextElement).textPainter.text!.style!; + }).toList(); + + expect(styles[0].fontSize, 12); + expect(styles[0].fontWeight, isNull); + expect(styles[0].fontStyle, isNull); + expect(styles[0].fontFamily, 'Helvetica'); + + expect(styles[1].fontSize, 10); + expect(styles[1].fontWeight, FontWeight.w900); + expect(styles[1].fontStyle, isNull); + expect(styles[1].fontFamily, 'Arial'); + + expect(styles[2].fontSize, 12); + expect(styles[2].fontWeight, isNull); + expect(styles[2].fontStyle, isNull); + expect(styles[2].fontFamily, 'Helvetica'); + + expect(styles[3].fontSize, 6); + expect(styles[3].fontWeight, isNull); + expect(styles[3].fontStyle, FontStyle.italic); + expect(styles[3].fontFamily, 'Arial'); + }); + }); +}