Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
19 changes: 12 additions & 7 deletions attributed_text/lib/src/attributed_text.dart
Original file line number Diff line number Diff line change
Expand Up @@ -421,18 +421,23 @@ class AttributedText {
final textEndCopyOffset =
(endOffset ?? length) - placeholdersBeforeStartOffset.length - placeholdersAfterStartBeforeEndOffset.length;

// The span marker offsets are based on the text with placeholders, so we need
// to copy the text with placeholders to ensure the span markers are correct.
final textWithPlaceholders = toPlainText();

// Note: -1 because copyText() uses an exclusive `start` and `end` but
// _copyAttributionRegion() uses an inclusive `start` and `end`.
final startCopyOffset = startOffset < _text.length ? startOffset : _text.length - 1;
int endCopyOffset;
final spansStartCopyOffset =
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear to me why we're creating new start and end offsets. Why aren't the existing start and end offsets correct? Also what does "spans" mean?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not clear to me why we're creating new start and end offsets. Why aren't the existing start and end offsets correct?

Because of the comment above, we need to adjust the offsets to make them inclusive. But I do agree that is not clear why the startCopyOffset could be greater than the text length.

Also what does "spans" mean?

Maybe it would be better to call this attributionStartCopyOffset, because this offset is passed to AttributedSpans.copyAttributionRegion to copy the attributions in the range.

Copy link
Contributor

Choose a reason for hiding this comment

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

Because of the comment above, we need to adjust the offsets to make them inclusive

Looking closer, was the existing variable just renamed? If so, I'm not sure that it needed a new name. Let me know if I'm missing something.

Could this PR be reduced to simply adding final textWithPlaceholders = toPlainText(); and using that instead of _text?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I was trying to make the variable name more clear, but I reverted the variable rename.

startOffset < textWithPlaceholders.length ? startOffset : textWithPlaceholders.length - 1;
int spansEndCopyOffset;
if (endOffset == startOffset) {
endCopyOffset = startCopyOffset;
spansEndCopyOffset = spansStartCopyOffset;
} else if (endOffset != null) {
endCopyOffset = endOffset - 1;
spansEndCopyOffset = endOffset - 1;
} else {
endCopyOffset = _text.length - 1;
spansEndCopyOffset = textWithPlaceholders.length - 1;
}
_log.fine('offsets, start: $startCopyOffset, end: $endCopyOffset');
_log.fine('offsets, start: $spansStartCopyOffset, end: $spansEndCopyOffset');

// Create placeholders for the copied region. The indices of the placeholders
// need to be reduced based on the text/placeholders cut out from the
Expand All @@ -444,7 +449,7 @@ class AttributedText {

return AttributedText(
_text.substring(textStartCopyOffset, textEndCopyOffset),
spans.copyAttributionRegion(startCopyOffset, endCopyOffset),
spans.copyAttributionRegion(spansStartCopyOffset, spansEndCopyOffset),
copiedPlaceholders,
);
}
Expand Down
40 changes: 40 additions & 0 deletions attributed_text/test/attributed_text_placeholders_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -420,6 +420,46 @@ void main() {
}),
);
});

test("empty text with leading placeholder (with attributions)", () {
// Create an empty text containing a placeholder with an attribution around it.
final text = AttributedText(
'',
AttributedSpans(
attributions: [
const SpanMarker(
attribution: _bold,
offset: 0,
markerType: SpanMarkerType.start,
),
const SpanMarker(
attribution: _bold,
offset: 0,
markerType: SpanMarkerType.end,
),
],
),
{
0: const _FakePlaceholder('leading'),
},
);

expect(
text.copyText(0),
AttributedText(
"",
AttributedSpans(
attributions: const [
SpanMarker(attribution: _bold, offset: 0, markerType: SpanMarkerType.start),
SpanMarker(attribution: _bold, offset: 0, markerType: SpanMarkerType.end),
],
),
{
0: const _FakePlaceholder("leading"),
},
),
);
});
});

group("copy and append >", () {
Expand Down
6 changes: 3 additions & 3 deletions super_editor/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,11 @@ dependencies:
flutter_test_robots: ^0.0.24
clock: ^1.1.1

#dependency_overrides:
dependency_overrides:
# Override to local mono-repo path so devs can test this repo
# against changes that they're making to other mono-repo packages
# attributed_text:
# path: ../attributed_text
attributed_text:
path: ../attributed_text
# super_text_layout:
# path: ../super_text_layout

Expand Down
54 changes: 54 additions & 0 deletions super_editor/test/super_editor/text_entry/inline_widgets_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,58 @@ void main() {
),
);
});

testWidgetsOnArbitraryDesktop("can insert an inline widget with attributions in an empty paragraph",
(tester) async {
final editor = await _pumpScaffold(tester);

// Insert an empty text containing a placeholder with an attribution around it.
editor.execute([
InsertStyledTextAtCaretRequest(
AttributedText(
'',
AttributedSpans(
attributions: [
const SpanMarker(
attribution: _emojiAttribution,
offset: 0,
markerType: SpanMarkerType.start,
),
const SpanMarker(
attribution: _emojiAttribution,
offset: 0,
markerType: SpanMarkerType.end,
),
],
),
{
0: const _TestPlaceholder(),
}),
),
]);
await tester.pump();

// Ensure we can type after the inline placeholder was added.
await tester.typeImeText('hello');

// Ensure the inline widget was kept, the text was inserted, and the attribution
// was not applied to the inserted characters.
expect(
SuperEditorInspector.findTextInComponent('1'),
AttributedText(
'hello',
AttributedSpans(
attributions: const [
SpanMarker(attribution: _emojiAttribution, offset: 0, markerType: SpanMarkerType.start),
SpanMarker(attribution: _emojiAttribution, offset: 0, markerType: SpanMarkerType.end),
],
),
{
0: const _TestPlaceholder(),
},
),
);
});
});
}

Expand Down Expand Up @@ -165,3 +217,5 @@ Widget? _buildInlineTestWidget(BuildContext context, TextStyle style, Object pla
class _TestPlaceholder {
const _TestPlaceholder();
}

const _emojiAttribution = NamedAttribution('emoji');
Loading