Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SuperEditor][SuperReader] - Reduce uses of node index queries in Document (Resolves #2434) #2435

Merged
merged 4 commits into from
Dec 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
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
149 changes: 44 additions & 105 deletions super_editor/lib/src/core/document_selection.dart
Original file line number Diff line number Diff line change
Expand Up @@ -150,21 +150,8 @@ class DocumentSelection extends DocumentRange {
return this;
}

final baseNode = document.getNodeById(base.nodeId)!;
final extentNode = document.getNodeById(extent.nodeId)!;

if (baseNode == extentNode) {
// The selection is expanded, but it sits within a single node.
final upstreamNodePosition = extentNode.selectUpstreamPosition(
base.nodePosition,
extent.nodePosition,
);
return DocumentSelection.collapsed(
position: extent.copyWith(nodePosition: upstreamNodePosition),
);
}

return document.getNodeIndexById(baseNode.id) < document.getNodeIndexById(extentNode.id)
final selectionAffinity = document.getAffinityForSelection(this);
return selectionAffinity == TextAffinity.downstream //
? DocumentSelection.collapsed(position: base)
: DocumentSelection.collapsed(position: extent);
}
Expand All @@ -189,23 +176,10 @@ class DocumentSelection extends DocumentRange {
return this;
}

final baseNode = document.getNodeById(base.nodeId)!;
final extentNode = document.getNodeById(extent.nodeId)!;

if (baseNode == extentNode) {
// The selection is expanded, but it sits within a single node.
final downstreamNodePosition = extentNode.selectDownstreamPosition(
base.nodePosition,
extent.nodePosition,
);
return DocumentSelection.collapsed(
position: extent.copyWith(nodePosition: downstreamNodePosition),
);
}

return document.getNodeIndexById(baseNode.id) > document.getNodeIndexById(extentNode.id)
? DocumentSelection.collapsed(position: base)
: DocumentSelection.collapsed(position: extent);
final selectionAffinity = document.getAffinityForSelection(this);
return selectionAffinity == TextAffinity.downstream //
? DocumentSelection.collapsed(position: extent)
: DocumentSelection.collapsed(position: base);
}

@override
Expand Down Expand Up @@ -326,6 +300,21 @@ extension InspectDocumentAffinity on Document {
return getAffinityBetween(base: range.start, extent: range.end);
}

TextAffinity getAffinityBetweenNodes(DocumentNode base, DocumentNode extent) {
return getAffinityForSelection(
DocumentSelection(
base: DocumentPosition(
nodeId: base.id,
nodePosition: base.beginningPosition,
),
extent: DocumentPosition(
nodeId: extent.id,
nodePosition: extent.beginningPosition,
),
),
);
}

/// Returns the affinity direction implied by the given [base] and [extent].
// TODO: Replace TextAffinity with a DocumentAffinity to avoid confusion.
TextAffinity getAffinityBetween({
Expand All @@ -336,19 +325,17 @@ extension InspectDocumentAffinity on Document {
if (baseNode == null) {
throw Exception('No such position in document: $base');
}
final baseIndex = getNodeIndexById(baseNode.id);

final extentNode = getNode(extent);
if (extentNode == null) {
throw Exception('No such position in document: $extent');
}
final extentIndex = getNodeIndexById(extentNode.id);

late TextAffinity affinity;
if (extentIndex > baseIndex) {
affinity = TextAffinity.downstream;
} else if (extentIndex < baseIndex) {
affinity = TextAffinity.upstream;
if (base.nodeId != extent.nodeId) {
affinity = getNodeIndexById(base.nodeId) < getNodeIndexById(extent.nodeId)
? TextAffinity.downstream
: TextAffinity.upstream;
} else {
// The selection is within the same node. Ask the node which position
// comes first.
Expand Down Expand Up @@ -384,20 +371,16 @@ extension InspectDocumentSelection on Document {
/// Given [docPosition1] and [docPosition2], returns the `DocumentPosition` that
/// appears first in the document.
DocumentPosition selectUpstreamPosition(DocumentPosition docPosition1, DocumentPosition docPosition2) {
final docPosition1Node = getNodeById(docPosition1.nodeId)!;
final docPosition1NodeIndex = getNodeIndexById(docPosition1Node.id);
final docPosition2Node = getNodeById(docPosition2.nodeId)!;
final docPosition2NodeIndex = getNodeIndexById(docPosition2Node.id);

if (docPosition1NodeIndex < docPosition2NodeIndex) {
return docPosition1;
} else if (docPosition2NodeIndex < docPosition1NodeIndex) {
return docPosition2;
if (docPosition1.nodeId != docPosition2.nodeId) {
final affinity = getAffinityBetween(base: docPosition1, extent: docPosition2);
return affinity == TextAffinity.downstream //
? docPosition1
: docPosition2;
}

// Both document positions are in the same node. Figure out which
// node position comes first.
final theNode = docPosition1Node;
final theNode = getNodeById(docPosition1.nodeId)!;
return theNode.selectUpstreamPosition(docPosition1.nodePosition, docPosition2.nodePosition) ==
docPosition1.nodePosition
? docPosition1
Expand All @@ -418,62 +401,18 @@ extension InspectDocumentSelection on Document {
return false;
}

final baseNode = getNodeById(selection.base.nodeId)!;
final baseNodeIndex = getNodeIndexById(baseNode.id);
final extentNode = getNodeById(selection.extent.nodeId)!;
final extentNodeIndex = getNodeIndexById(extentNode.id);

final upstreamNode = baseNodeIndex < extentNodeIndex ? baseNode : extentNode;
final upstreamNodeIndex = baseNodeIndex < extentNodeIndex ? baseNodeIndex : extentNodeIndex;
final downstreamNode = baseNodeIndex < extentNodeIndex ? extentNode : baseNode;
final downstreamNodeIndex = baseNodeIndex < extentNodeIndex ? extentNodeIndex : baseNodeIndex;

final positionNodeIndex = getNodeIndexById(position.nodeId);

if (upstreamNodeIndex < positionNodeIndex && positionNodeIndex < downstreamNodeIndex) {
// The given position is sandwiched between two other nodes that form
// the bounds of the selection. Therefore, the position is definitely within
// the selection.
return true;
}

if (positionNodeIndex == upstreamNodeIndex) {
final upstreamPosition = selectUpstreamPosition(selection.base, selection.extent);
final downstreamPosition = upstreamPosition == selection.base ? selection.extent : selection.base;

// This is the furthest a position could sit in the upstream node
// and still contain the given position. Keep in mind that the
// upstream position, downstream position, and given position may
// all reside in the same node (in fact, they probably do).
final downstreamCap =
upstreamNodeIndex == downstreamNodeIndex ? downstreamPosition.nodePosition : upstreamNode.endPosition;

// If and only if the given position comes after the upstream position,
// and before the downstream cap, then the position is within the selection.
return upstreamNode.selectDownstreamPosition(upstreamPosition.nodePosition, position.nodePosition) ==
upstreamNode.selectUpstreamPosition(position.nodePosition, downstreamCap);
}

if (positionNodeIndex == downstreamNodeIndex) {
final upstreamPosition = selectUpstreamPosition(selection.base, selection.extent);
final downstreamPosition = upstreamPosition == selection.base ? selection.extent : selection.base;

// This is the furthest upstream that a position could sit in the
// downstream node and still contain the given position. Keep in
// mind that the upstream position, downstream position, and given
// position may all reside in the same node (in fact, they probably do).
final upstreamCap =
downstreamNodeIndex == upstreamNodeIndex ? upstreamPosition.nodePosition : downstreamNode.beginningPosition;

// If and only if the given position comes before the downstream position,
// and after the upstream cap, then the position is within the selection.
return downstreamNode.selectDownstreamPosition(upstreamCap, position.nodePosition) ==
downstreamNode.selectUpstreamPosition(position.nodePosition, downstreamPosition.nodePosition);
}

// If we got here, then the position is either before the upstream
// selection boundary, or after the downstream selection boundary.
// Either way, the position is not in the selection.
return false;
final selectionAffinity = getAffinityForSelection(selection);
final upstreamPosition = selectionAffinity == TextAffinity.downstream ? selection.base : selection.extent;
final downstreamPosition = selectionAffinity == TextAffinity.downstream ? selection.extent : selection.base;

// The selection contains the position if the ordering is as follows:
//
// selection start <= position <= selection end
//
// Another way of stating this relationship is that there's a downstream
// affinity from selection start to the position, and from the position to
// the selection end.
return getAffinityBetween(base: upstreamPosition, extent: position) == TextAffinity.downstream &&
getAffinityBetween(base: position, extent: downstreamPosition) == TextAffinity.downstream;
}
}
16 changes: 13 additions & 3 deletions super_editor/lib/src/core/editor.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1278,7 +1278,7 @@ class MutableDocument with Iterable<DocumentNode> implements Document, Editable
}
}

/// Returns [true] if the content of the [other] [Document] is equivalent
/// Returns `true` if the content of the [other] [Document] is equivalent
/// to the content of this [Document].
///
/// Content equivalency compares types of content nodes, and the content
Expand All @@ -1289,11 +1289,21 @@ class MutableDocument with Iterable<DocumentNode> implements Document, Editable
if (_nodes.length != other.nodeCount) {
return false;
}
if (isEmpty) {
// Both documents are empty, and therefore are equivalent.
return true;
}

DocumentNode? thisDocNode = first;
DocumentNode? otherDocNode = other.first;

for (int i = 0; i < _nodes.length; ++i) {
if (!_nodes[i].hasEquivalentContent(other.getNodeAt(i)!)) {
while (thisDocNode != null && otherDocNode != null) {
if (!thisDocNode.hasEquivalentContent(otherDocNode)) {
return false;
}

thisDocNode = getNodeAfter(thisDocNode);
otherDocNode = other.getNodeAfter(otherDocNode);
}

return true;
Expand Down
4 changes: 2 additions & 2 deletions super_editor/lib/src/core/styles.dart
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ class _FirstBlockMatcher implements _BlockMatcher {

@override
bool matches(Document document, DocumentNode node) {
return document.getNodeIndexById(node.id) == 0;
return document.getNodeById(node.id) == document.firstOrNull;
}
}

Expand All @@ -216,7 +216,7 @@ class _LastBlockMatcher implements _BlockMatcher {

@override
bool matches(Document document, DocumentNode node) {
return document.getNodeIndexById(node.id) == document.nodeCount - 1;
return document.getNodeById(node.id) == document.lastOrNull;
}
}

Expand Down
24 changes: 13 additions & 11 deletions super_editor/lib/src/default_editor/common_editor_operations.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1404,31 +1404,33 @@ class CommonEditorOperations {
if (baseNode == null) {
throw Exception('Failed to _getDocumentPositionAfterDeletion because the base node no longer exists.');
}
final baseNodeIndex = document.getNodeIndexById(baseNode.id);

final extentPosition = selection.extent;
final extentNode = document.getNode(extentPosition);
if (extentNode == null) {
throw Exception('Failed to _getDocumentPositionAfterDeletion because the extent node no longer exists.');
}
final extentNodeIndex = document.getNodeIndexById(extentNode.id);

final topNodeIndex = min(baseNodeIndex, extentNodeIndex);
final topNode = document.getNodeAt(topNodeIndex)!;
final topNodePosition = baseNodeIndex < extentNodeIndex ? basePosition.nodePosition : extentPosition.nodePosition;
final selectionAffinity = document.getAffinityForSelection(selection);
final topPosition = selectionAffinity == TextAffinity.downstream //
? selection.base
: selection.extent;
final topNodePosition = topPosition.nodePosition;
final topNode = document.getNodeById(topPosition.nodeId)!;

final bottomNodeIndex = max(baseNodeIndex, extentNodeIndex);
final bottomNode = document.getNodeAt(bottomNodeIndex)!;
final bottomNodePosition =
baseNodeIndex < extentNodeIndex ? extentPosition.nodePosition : basePosition.nodePosition;
final bottomPosition = selectionAffinity == TextAffinity.downstream //
? selection.extent
: selection.base;
final bottomNodePosition = bottomPosition.nodePosition;
final bottomNode = document.getNodeById(bottomPosition.nodeId)!;

final normalizedRange = selection.normalize(document);
final nodes = document.getNodesInside(normalizedRange.start, normalizedRange.end);
final firstDeletableNodeId = nodes.firstWhereOrNull((node) => node.isDeletable)?.id;

DocumentPosition newSelectionPosition;

if (baseNodeIndex != extentNodeIndex) {
if (topPosition.nodeId != bottomPosition.nodeId) {
if (topNodePosition == topNode.beginningPosition && bottomNodePosition == bottomNode.endPosition) {
// All deletable nodes in the selection will be deleted. Assume that one of the
// nodes will be retained and converted into a paragraph, if it's not
Expand Down Expand Up @@ -1469,7 +1471,7 @@ class CommonEditorOperations {
// those nodes will remain.

// The caret should end up at the base position
newSelectionPosition = baseNodeIndex <= extentNodeIndex ? selection.base : selection.extent;
newSelectionPosition = selectionAffinity == TextAffinity.downstream ? selection.base : selection.extent;
}
} else {
// Selection is within a single node.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -388,17 +388,14 @@ class TextDeltasDocumentEditor {
editorImeLog.fine("Doc selection to delete: $docSelectionToDelete");

if (docSelectionToDelete == null) {
final selectedNodeIndex = document.getNodeIndexById(
selection.value!.extent.nodeId,
);
// The user is trying to delete upstream at the start of a node.
// This action requires intervention because the IME doesn't know
// that there's more content before this node. Instruct the editor
// to run a delete action upstream, which will take the desired
// "backspace" behavior at the start of this node.
editor.execute([
DeleteUpstreamAtBeginningOfNodeRequest(
document.getNodeAt(selectedNodeIndex)!,
document.getNodeById(selection.value!.extent.nodeId)!,
),
]);
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,17 +164,16 @@ class SingleColumnLayoutPresenter {
// The document changed. All view models were invalidated. Create a
// new base document view model.
final viewModels = <SingleColumnLayoutComponentViewModel>[];
for (int i = 0; i < _document.nodeCount; i += 1) {
for (final node in _document) {
SingleColumnLayoutComponentViewModel? viewModel;
for (final builder in _componentBuilders) {
viewModel = builder.createViewModel(_document, _document.getNodeAt(i)!);
viewModel = builder.createViewModel(_document, node);
if (viewModel != null) {
break;
}
}
if (viewModel == null) {
throw Exception(
"Couldn't find styler to create component for document node: ${_document.getNodeAt(i)!.runtimeType}");
throw Exception("Couldn't find styler to create component for document node: ${node.runtimeType}");
}
viewModels.add(viewModel);
}
Expand Down
39 changes: 25 additions & 14 deletions super_editor/lib/src/default_editor/multi_node_editing.dart
Original file line number Diff line number Diff line change
Expand Up @@ -923,28 +923,39 @@ class DeleteContentCommand extends EditCommand {
required DocumentNode startNode,
required DocumentNode endNode,
}) {
if (startNode.id == endNode.id) {
// The start and end nodes are the same. Nothing to delete.
return [];
}

// Delete all nodes between the first node and the last node.
final startIndex = document.getNodeIndexById(startNode.id);
final endIndex = document.getNodeIndexById(endNode.id);
if (document.getAffinityBetweenNodes(startNode, endNode) != TextAffinity.downstream) {
throw Exception(
"Tried to delete the nodes between a start and end node, but the start node doesn't appear before the end node. Start: ${startNode.id}, End: ${endNode.id}.",
);
}

_log.log('_deleteNodesBetweenFirstAndLast', ' - start node index: $startIndex');
_log.log('_deleteNodesBetweenFirstAndLast', ' - end node index: $endIndex');
_log.log('_deleteNodesBetweenFirstAndLast', ' - start node: ${startNode.id}');
_log.log('_deleteNodesBetweenFirstAndLast', ' - end node: ${endNode.id}');
_log.log('_deleteNodesBetweenFirstAndLast', ' - initially ${document.nodeCount} nodes');

// Remove nodes from last to first so that indices don't get
// screwed up during removal.
final changes = <EditEvent>[];
for (int i = endIndex - 1; i > startIndex; --i) {
_log.log('_deleteNodesBetweenFirstAndLast', ' - deleting node $i: ${document.getNodeAt(i)?.id}');
final removedNode = document.getNodeAt(i)!;
if (!removedNode.isDeletable) {
// This node is not deletable. Ignore it.
continue;
var nodeToDelete = document.getNodeAfter(startNode);
while (nodeToDelete != null && nodeToDelete != endNode) {
_log.log('_deleteNodesBetweenFirstAndLast', ' - deleting node: ${nodeToDelete.id}');
final nextNode = document.getNodeAfter(nodeToDelete);
if (nodeToDelete.isDeletable) {
// This node is deletable, so delete it.
changes.add(DocumentEdit(
NodeRemovedEvent(nodeToDelete.id, nodeToDelete),
));
document.deleteNode(nodeToDelete);
}
changes.add(DocumentEdit(
NodeRemovedEvent(removedNode.id, removedNode),
));
document.deleteNodeAt(i);

// Move to the next node.
nodeToDelete = nextNode;
}
return changes;
}
Expand Down
Loading
Loading