Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Skwasm] Correctly handle paragraphs with empty text. #51695

Merged
auto-submit merged 2 commits into from
Mar 27, 2024
Merged

[Skwasm] Correctly handle paragraphs with empty text. #51695

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
32b2f87
[Skwasm] Correctly handle paragraphs with empty text.
eyebrowsoffire Mar 27, 2024
5c7a632
Address comments.
eyebrowsoffire Mar 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 22 additions & 18 deletions lib/web_ui/lib/src/engine/skwasm/skwasm_impl/paragraph.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -940,27 +940,31 @@ class SkwasmParagraphBuilder extends SkwasmObjectWrapper<RawParagraphBuilder> im
// just create both up front here.
final Pointer<Uint32> outSize = scope.allocUint32Array(1);
final Pointer<Uint8> utf8Data = paragraphBuilderGetUtf8Text(handle, outSize);

final String text;
final JSString jsText;
if (utf8Data == nullptr) {
return;
text = '';
jsText = ''.toJS;
} else {
// TODO(jacksongardner): We could make a subclass of `List<int>` here to
// avoid this copy.

@mdebbar mdebbar Mar 27, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to suggest using Iterable<int>.generate(...) instead, but then realized that utf8.decode(...) takes a List<int>. I wonder why.

@yjbanov yjbanov Mar 27, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mdebbar mdebbar Mar 27, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's probably true for dart2js since raw List are basically JS arrays under the hood (IIUC). But is it the same for wasm?

@eyebrowsoffire eyebrowsoffire Mar 27, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

final List<int> codeUnitList = List<int>.generate(
outSize.value,
(int index) => utf8Data[index]
);
text = utf8.decode(codeUnitList);
jsText = _utf8Decoder.decode(
// In an ideal world we would just use a subview of wasm memory rather
// than a slice, but the TextDecoder API doesn't work on shared buffer
// sources yet.
// See https://bugs.chromium.org/p/chromium/issues/detail?id=1012656
createUint8ArrayFromBuffer(skwasmInstance.wasmMemory.buffer).slice(
utf8Data.address.toJS,
(utf8Data.address + outSize.value).toJS
));
}

// TODO(jacksongardner): We could make a subclass of `List<int>` here to
// avoid this copy.
final List<int> codeUnitList = List<int>.generate(
outSize.value,
(int index) => utf8Data[index]
);
final String text = utf8.decode(codeUnitList);
final JSString jsText = _utf8Decoder.decode(
// In an ideal world we would just use a subview of wasm memory rather
// than a slice, but the TextDecoder API doesn't work on shared buffer
// sources yet.
// See https://bugs.chromium.org/p/chromium/issues/detail?id=1012656
createUint8ArrayFromBuffer(skwasmInstance.wasmMemory.buffer).slice(
utf8Data.address.toJS,
(utf8Data.address + outSize.value).toJS
));

_addGraphemeBreakData(text, jsText);
_addWordBreakData(text, jsText);
_addLineBreakData(text, jsText);
Expand Down
2 changes: 1 addition & 1 deletion lib/web_ui/test/ui/line_metrics_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

@yjbanov yjbanov Mar 27, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We desperately need a lint that prevents this.

@yjbanov yjbanov Mar 27, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, solo is marked with @Deprecated. We should not have been able to check it in 🤔

@eyebrowsoffire eyebrowsoffire Mar 27, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

});
}
7 changes: 7 additions & 0 deletions lib/web_ui/test/ui/paragraph_builder_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,4 +39,11 @@ Future<void> testMain() async {

expect(() => builder.build(), returnsNormally);
});

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));
});
Comment on lines +43 to +51

@mdebbar mdebbar Mar 27, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
);

@yjbanov yjbanov Mar 27, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's probably some use in actually checking the layout properties of an empty paragraph.

@eyebrowsoffire eyebrowsoffire Mar 27, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}