[Skwasm] Correctly handle paragraphs with empty text. #51695
Conversation
| // TODO(jacksongardner): We could make a subclass of `List<int>` here to | ||
| // avoid this copy. |
There was a problem hiding this comment.
I was going to suggest using Iterable<int>.generate(...) instead, but then realized that utf8.decode(...) takes a List<int>. I wonder why.
There was a problem hiding this comment.
Typically anything but a raw List would be many times slower. I'm not surprised utf8 prefers lists. Honestly, this is fine. I wouldn't even leave a TODO.
There was a problem hiding this comment.
That's probably true for dart2js since raw List are basically JS arrays under the hood (IIUC). But is it the same for wasm?
There was a problem hiding this comment.
It's not the same for wasm, List is definitely polymorphic. A while back I actually did try making a subclass of List<int> that reads from an ffi pointer, and it actually was slightly slower. I think I'll just remove this TODO.
| test('build and layout a paragraph with an empty addText', () { | ||
| final ParagraphBuilder builder = ParagraphBuilder(ParagraphStyle()); | ||
| builder.addText(''); | ||
| final Paragraph paragraph = builder.build(); | ||
| paragraph.layout(const ParagraphConstraints(width: double.infinity)); | ||
| }); |
There was a problem hiding this comment.
It's a bit odd to have a test with no expectations. Can we add one to emphasize what the test is checking?
e.g.
expect(
() => paragraph.layout(const ParagraphConstraints(width: double.infinity)),
returnsNormally,
);There was a problem hiding this comment.
There's probably some use in actually checking the layout properties of an empty paragraph.
There was a problem hiding this comment.
I have a separate test for a straight up empty paragraph (with no addText at all) in the line_metrics_test.dart file. However, that one is skipped on the html renderer, because the properties of an empty paragraph actually differ between renderers. See flutter/flutter#143331
So I'd rather just do one that doesn't read any metrics. But I think doing the returnsNormally thing that Mouad suggested is a good idea.
| @@ -176,5 +176,5 @@ Future<void> testMain() async { | |||
|
|
|||
| // In Roboto, the width should be 11 here. In the test font, it would be square (16 points) | |||
| expect(metrics!.width, 11); | |||
| }, solo: true); | |||
There was a problem hiding this comment.
We desperately need a lint that prevents this.
There was a problem hiding this comment.
Actually, solo is marked with @Deprecated. We should not have been able to check it in 🤔
There was a problem hiding this comment.
Yeah I was wondering about this. We used to have a lint I thought, but it disappeared apparently. I was surprised to find I had been able to check this in too
eyebrowsoffire
added
the
autosubmit
…145862) flutter/engine@d656625...73c145c 2024-03-27 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] Use the scissor to limit all draws by clip coverage. (#51698)" (flutter/engine#51728) 2024-03-27 jacksongardner@google.com [Skwasm] Correctly handle paragraphs with empty text. (flutter/engine#51695) 2024-03-27 skia-flutter-autoroll@skia.org Roll Dart SDK from a600b67424a8 to 291217c1d399 (4 revisions) (flutter/engine#51716) 2024-03-27 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Fail pre-submit if a negative image is encountered as part of `goldctl imgtest add`. (#51685)" (flutter/engine#51718) 2024-03-27 bdero@google.com [Impeller] Use the scissor to limit all draws by clip coverage. (flutter/engine#51698) 2024-03-27 1961493+harryterkelsen@users.noreply.github.com [canvaskit] Fix color filter for dst and dstIn (flutter/engine#51693) 2024-03-27 jonahwilliams@google.com [Impeller] add missing null check. (flutter/engine#51711) 2024-03-27 jonahwilliams@google.com [Impeller] fix remaining Validation errors. (flutter/engine#51692) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jacksongardner@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Instead of just returning, if our paragraph builder has empty text, we still need to generate a set of line breaks for skia to use. Otherwise, it will actually cause subtle memory access errors.