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.

Sign up for GitHub

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] Fix typing lag in large documents (Resolves #751) #839

Merged
merged 15 commits into from
Nov 2, 2022
Merged

[SuperEditor] Fix typing lag in large documents (Resolves #751) #839

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
efda3fa
[SuperEditor] Fix typing lag in large documents (Resolves #751)
angelosilvestre Oct 26, 2022
350b644
Attempt to optimize mouse selection
angelosilvestre Oct 27, 2022
92d9d8e
PR Update
angelosilvestre Oct 27, 2022
e1d47d6
Replace List.unmodifiable with UnmodifiableListView.
angelosilvestre Oct 27, 2022
c65a724
Mouse dragging performance improvement
angelosilvestre Oct 29, 2022
d9660d7
Add Tests
angelosilvestre Oct 29, 2022
f5a76ca
File missing from last commit
angelosilvestre Oct 29, 2022
1ba2526
Add dart doc
angelosilvestre Oct 31, 2022
1268ae6
PR updates
angelosilvestre Oct 31, 2022
423ae1d
PR updates
angelosilvestre Nov 1, 2022
301bf93
Rename method
angelosilvestre Nov 1, 2022
d0488fc
Change attribution commands
angelosilvestre Nov 1, 2022
b9cc1b2
Add equality comparison in getNodeIndex
angelosilvestre Nov 1, 2022
018840a
Update comment
angelosilvestre Nov 2, 2022
20ddc06
Deprecate getNodeIndex
angelosilvestre Nov 2, 2022
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
2 changes: 0 additions & 2 deletions super_editor/example/lib/demos/example_editor/_task.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,6 @@ class TaskComponentViewModel extends SingleColumnLayoutComponentViewModel with T
isComplete == other.isComplete &&
setComplete == other.setComplete &&
text == other.text &&
textStyleBuilder == other.textStyleBuilder &&
textDirection == other.textDirection &&
textAlignment == other.textAlignment &&
selection == other.selection &&
Expand All @@ -192,7 +191,6 @@ class TaskComponentViewModel extends SingleColumnLayoutComponentViewModel with T
isComplete.hashCode ^
setComplete.hashCode ^
text.hashCode ^
textStyleBuilder.hashCode ^
textDirection.hashCode ^
textAlignment.hashCode ^
selection.hashCode ^
Expand Down
103 changes: 76 additions & 27 deletions super_editor/lib/src/core/document_editor.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import 'dart:math';

import 'package:collection/collection.dart';
import 'package:flutter/foundation.dart';
import 'package:flutter/material.dart';
import 'package:super_editor/src/core/document_selection.dart';
import 'package:super_editor/src/infrastructure/_logging.dart';
Expand Down Expand Up @@ -133,22 +132,29 @@ class MutableDocument with ChangeNotifier implements Document {
MutableDocument({
List<DocumentNode>? nodes,
}) : _nodes = nodes ?? [] {
// Register listeners for all initial nodes.
for (final node in _nodes) {
// Register listeners for all initial nodes and populates the node maps.
for (int i = 0; i < _nodes.length; i++) {
final node = _nodes[i];
node.addListener(_forwardNodeChange);
_nodeIndicesById[node.id] = i;
_nodesById[node.id] = node;
}
}

final List<DocumentNode> _nodes;

@override
List<DocumentNode> get nodes => _nodes;
List<DocumentNode> get nodes => List.unmodifiable(_nodes);

/// Maps a node id to its index in the node list.
final Map<String, int> _nodeIndicesById = {};

/// Maps a node id to its node.
final Map<String, DocumentNode> _nodesById = {};

@override
DocumentNode? getNodeById(String nodeId) {
return _nodes.firstWhereOrNull(
(element) => element.id == nodeId,
);
return _nodesById[nodeId];
}

@override
Expand All @@ -162,7 +168,10 @@ class MutableDocument with ChangeNotifier implements Document {

@override
int getNodeIndex(DocumentNode node) {
return _nodes.indexOf(node);
// We always update _nodeIndicesById when the node list changes.
angelosilvestre marked this conversation as resolved.
Show resolved Hide resolved
// If the id isn't present in the map it means the node isn't present
// in the document.
return _nodeIndicesById[node.id] ?? -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is changing the behavior of the method slightly. It will now return an index if you pass a node that shares an id with one in the document even if they are otherwise unequal. Additionally, if someone implemented custom nodes with overridden == behavior, this method may now return -1 instead of a valid index.

In my implementation I used the cache to quickly look up the node and then performed the comparison to maintain the existing behavior. Code here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I updated to add the equality check.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought about this a bit more. Why would there ever be 2 nodes with the same ID and different content? The point of the ID is to uniquely refer to a node within a document...

Copy link
Contributor

Choose a reason for hiding this comment

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

@jmatth can you let me know when/why this change would be a problem?

Copy link
Contributor

Choose a reason for hiding this comment

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

I could trivially construct two nodes with the same ID that are otherwise unequal, store one in a MutableDocument, then pass the second one to getNodeIndex and get a valid index back. I find that result unexpected. Best example I can think of for why this would happen is converting between node types, such as a paragraph to a list item or similar, but I don't actually have any real world examples. If you don't think that's a reasonable use case to accommodate then this method should just be removed entirely, because getNodeIndexById also exists, its signature makes it much clearer what the criteria for a match are, and without the equality check the two methods serve the exact same purpose.

Copy link
Contributor

Choose a reason for hiding this comment

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

That said, if removal looks acceptable, there's probably no reason to make that breaking change now. We're trying to land a semi-stable release after this PR. So @angelosilvestre if you think removal looks OK, then add a "deprecated" annotation to the method and we'll merge it in as-is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will take a look.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@matthew-carroll We are using this method in a number of places, but we could change them to use getNodeIndexById. Should I add the deprecated annotation and change the usages?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you've inspected all of those locations, and you're comfortable that changing them won't be a big deal, then sure, you can go ahead and deprecate this method. Please be sure to include a deprecation message that points to the other method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I marked getNodeIndex as deprecated and changed the places which use this method.

}

@override
Expand All @@ -180,12 +189,11 @@ class MutableDocument with ChangeNotifier implements Document {
@override
DocumentNode? getNodeAfter(DocumentNode node) {
final nodeIndex = getNodeIndex(node);
return nodeIndex >= 0 && nodeIndex < nodes.length - 1 ? getNodeAt(nodeIndex + 1) : null;
return nodeIndex >= 0 && nodeIndex < _nodes.length - 1 ? getNodeAt(nodeIndex + 1) : null;
}

@override
DocumentNode? getNode(DocumentPosition position) =>
_nodes.firstWhereOrNull((element) => element.id == position.nodeId);
DocumentNode? getNode(DocumentPosition position) => getNodeById(position.nodeId);

@override
DocumentRange getRangeBetween(DocumentPosition position1, DocumentPosition position2) {
Expand All @@ -202,13 +210,13 @@ class MutableDocument with ChangeNotifier implements Document {
if (node1 == null) {
throw Exception('No such position in document: $position1');
}
final index1 = _nodes.indexOf(node1);
final index1 = getNodeIndex(node1);

final node2 = getNode(position2);
if (node2 == null) {
throw Exception('No such position in document: $position2');
}
final index2 = _nodes.indexOf(node2);
final index2 = getNodeIndex(node2);

final from = min(index1, index2);
final to = max(index1, index2);
Expand All @@ -218,9 +226,13 @@ class MutableDocument with ChangeNotifier implements Document {

/// Inserts the given [node] into the [Document] at the given [index].
void insertNodeAt(int index, DocumentNode node) {
if (index <= nodes.length) {
nodes.insert(index, node);
if (index <= _nodes.length) {
_nodes.insert(index, node);
node.addListener(_forwardNodeChange);

// The node list changed, we need to update the map to consider the new indices.
_updateNodeIdMappings();

notifyListeners();
}
}
Expand All @@ -230,9 +242,13 @@ class MutableDocument with ChangeNotifier implements Document {
required DocumentNode existingNode,
required DocumentNode newNode,
}) {
final nodeIndex = nodes.indexOf(existingNode);
nodes.insert(nodeIndex, newNode);
final nodeIndex = getNodeIndex(existingNode);
_nodes.insert(nodeIndex, newNode);
newNode.addListener(_forwardNodeChange);

// The node list changed, we need to update the map to consider the new indices.
_updateNodeIdMappings();

notifyListeners();
}

Expand All @@ -241,19 +257,27 @@ class MutableDocument with ChangeNotifier implements Document {
required DocumentNode existingNode,
required DocumentNode newNode,
}) {
final nodeIndex = nodes.indexOf(existingNode);
if (nodeIndex >= 0 && nodeIndex < nodes.length) {
nodes.insert(nodeIndex + 1, newNode);
final nodeIndex = getNodeIndex(existingNode);
if (nodeIndex >= 0 && nodeIndex < _nodes.length) {
_nodes.insert(nodeIndex + 1, newNode);
newNode.addListener(_forwardNodeChange);

// The node list changed, we need to update the map to consider the new indices.
_updateNodeIdMappings();

notifyListeners();
}
}

/// Deletes the node at the given [index].
void deleteNodeAt(int index) {
if (index >= 0 && index < nodes.length) {
final removedNode = nodes.removeAt(index);
if (index >= 0 && index < _nodes.length) {
final removedNode = _nodes.removeAt(index);
removedNode.removeListener(_forwardNodeChange);

// The node list changed, we need to update the map to consider the new indices.
_updateNodeIdMappings();

notifyListeners();
} else {
editorDocLog.warning('Could not delete node. Index out of range: $index');
Expand All @@ -265,7 +289,11 @@ class MutableDocument with ChangeNotifier implements Document {
bool isRemoved = false;

node.removeListener(_forwardNodeChange);
isRemoved = nodes.remove(node);

isRemoved = _nodes.remove(node);

// The node list changed, we need to update the map to consider the new indices.
_updateNodeIdMappings();

notifyListeners();

Expand All @@ -282,8 +310,12 @@ class MutableDocument with ChangeNotifier implements Document {
throw Exception('Could not find node with nodeId: $nodeId');
}

if (nodes.remove(node)) {
nodes.insert(targetIndex, node);
if (_nodes.remove(node)) {
_nodes.insert(targetIndex, node);

// The node list changed, we need to update the map to consider the new indices.
angelosilvestre marked this conversation as resolved.
Show resolved Hide resolved
_updateNodeIdMappings();

notifyListeners();
}
}
Expand All @@ -302,6 +334,9 @@ class MutableDocument with ChangeNotifier implements Document {
newNode.addListener(_forwardNodeChange);
_nodes.insert(index, newNode);

// The node list changed, we need to update the map to consider the new indices.
angelosilvestre marked this conversation as resolved.
Show resolved Hide resolved
_updateNodeIdMappings();

notifyListeners();
} else {
throw Exception('Could not find oldNode: ${oldNode.id}');
Expand All @@ -320,19 +355,33 @@ class MutableDocument with ChangeNotifier implements Document {
/// ignores the runtime type of the [Document], itself.
@override
bool hasEquivalentContent(Document other) {
if (_nodes.length != other.nodes.length) {
final otherNodes = other.nodes;
if (_nodes.length != otherNodes.length) {
return false;
}

for (int i = 0; i < _nodes.length; ++i) {
if (!_nodes[i].hasEquivalentContent(other.nodes[i])) {
if (!_nodes[i].hasEquivalentContent(otherNodes[i])) {
return false;
}
}

return true;
}

/// Updates all the maps which use the node id as the key.
///
/// All the maps are cleared and re-populated.
void _updateNodeIdMappings() {
angelosilvestre marked this conversation as resolved.
Show resolved Hide resolved
_nodeIndicesById.clear();
_nodesById.clear();
for (int i = 0; i < _nodes.length; i++) {
final node = _nodes[i];
_nodeIndicesById[node.id] = i;
_nodesById[node.id] = node;
}
}

@override
bool operator ==(Object other) =>
identical(this, other) ||
Expand Down
2 changes: 0 additions & 2 deletions super_editor/lib/src/default_editor/blockquote.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,6 @@ class BlockquoteComponentViewModel extends SingleColumnLayoutComponentViewModel
runtimeType == other.runtimeType &&
nodeId == other.nodeId &&
text == other.text &&
textStyleBuilder == other.textStyleBuilder &&
textDirection == other.textDirection &&
textAlignment == other.textAlignment &&
backgroundColor == other.backgroundColor &&
Expand All @@ -161,7 +160,6 @@ class BlockquoteComponentViewModel extends SingleColumnLayoutComponentViewModel
super.hashCode ^
nodeId.hashCode ^
text.hashCode ^
textStyleBuilder.hashCode ^
textDirection.hashCode ^
textAlignment.hashCode ^
backgroundColor.hashCode ^
Expand Down
9 changes: 8 additions & 1 deletion super_editor/lib/src/default_editor/layout_single_column/_layout.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,6 +367,13 @@ class _SingleColumnDocumentLayoutState extends State<SingleColumnDocumentLayout>
bottomNodeId = _nodeIdsToComponentKeys.entries.firstWhere((element) => element.value == componentKey).key;
bottomNodeBasePosition = _getNodePositionForComponentOffset(component, componentBaseOffset);
bottomNodeExtentPosition = _getNodePositionForComponentOffset(component, componentExtentOffset);
} else if (topNodeId != null) {
// We already found an overlapping component and the current component doesn't
// overlap with the region.
// Because we're iterating through components from top to bottom,
// it means that there isn't any other component which will overlap,
// so we can skip the rest of the list.
break;
}
}

Expand Down Expand Up @@ -605,7 +612,7 @@ class _SingleColumnDocumentLayoutState extends State<SingleColumnDocumentLayout>
DocumentPosition? findLastSelectablePosition() {
NodePosition? nodePosition;
String? nodeId;

for (int i = _topToBottomComponentKeys.length - 1; i >= 0; i--) {
final componentKey = _topToBottomComponentKeys[i];
final component = componentKey.currentState as DocumentComponent;
Expand Down
2 changes: 0 additions & 2 deletions super_editor/lib/src/default_editor/list_items.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,6 @@ class ListItemComponentViewModel extends SingleColumnLayoutComponentViewModel wi
ordinalValue == other.ordinalValue &&
indent == other.indent &&
text == other.text &&
textStyleBuilder == other.textStyleBuilder &&
textDirection == other.textDirection &&
selection == other.selection &&
selectionColor == other.selectionColor;
Expand All @@ -244,7 +243,6 @@ class ListItemComponentViewModel extends SingleColumnLayoutComponentViewModel wi
ordinalValue.hashCode ^
indent.hashCode ^
text.hashCode ^
textStyleBuilder.hashCode ^
textDirection.hashCode ^
selection.hashCode ^
selectionColor.hashCode;
Expand Down
2 changes: 0 additions & 2 deletions super_editor/lib/src/default_editor/paragraph.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,6 @@ class ParagraphComponentViewModel extends SingleColumnLayoutComponentViewModel w
nodeId == other.nodeId &&
blockType == other.blockType &&
text == other.text &&
textStyleBuilder == other.textStyleBuilder &&
textDirection == other.textDirection &&
textAlignment == other.textAlignment &&
selection == other.selection &&
Expand All @@ -175,7 +174,6 @@ class ParagraphComponentViewModel extends SingleColumnLayoutComponentViewModel w
nodeId.hashCode ^
blockType.hashCode ^
text.hashCode ^
textStyleBuilder.hashCode ^
textDirection.hashCode ^
textAlignment.hashCode ^
selection.hashCode ^
Expand Down
48 changes: 39 additions & 9 deletions super_editor/lib/src/default_editor/text.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -887,9 +887,19 @@ class AddTextAttributionsCommand implements EditorCommand {
final node = entry.key;
final range = entry.value.toSpanRange();
editorDocLog.info(' - adding attribution: $attribution. Range: $range');
node.text.addAttribution(
attribution,
range,

// During style phase, we check which components changed, so we only re-style those components.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you file an issue for this? We should probably have a way to compare content.

Also, please add a TODO to each of these comments that references the issue that you file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe this will change in #748 ? I will reference this issue then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nevermind, I was confused with another statement.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have a way to compare content. The AttributedText equality operator is defined as:

@override
  bool operator ==(Object other) {
    return identical(this, other) ||
        other is AttributedText && runtimeType == other.runtimeType && text == other.text && spans == other.spans;
  }

If we just toggle the attributions in the same instance, the first condition will always be true.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you reframe this as an action item? I'm not sure what to do with the above information...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ParagraphComponentViewModel compares its AttributedText as part of its equality operator (text == other.text).

Before the change, we were only modifying the contents of the same AttributedText that was referenced in the viewmodel. The contents changed, but as it's referencing the same object, the equality operator returns true, as identical(this, other) is true.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. Let's change the comment to:

// Create a new AttributedText with updated attribution spans, so that the presentation system can
// see that we made a change, and re-renders the text in the document.

Let's change the implementation to:

node.text = AttributedText(
  text: node.text.text,
  spans: node.text.spans.copy()..addAttribution(
    newAttribution: attribution,
    start: range.start,
    end: range.end,
  ),
);

// If we just add the attribution on the same instance of AttributedText, we can't detect the change,
// because comparing the same instance will always evaluate to true.
final spans = node.text.spans.copy();
spans.addAttribution(
newAttribution: attribution,
start: range.start,
end: range.end,
);
node.text = AttributedText(
text: node.text.text,
spans: spans,
);
}
}
Expand Down Expand Up @@ -975,9 +985,19 @@ class RemoveTextAttributionsCommand implements EditorCommand {
final node = entry.key;
final range = entry.value.toSpanRange();
editorDocLog.info(' - removing attribution: $attribution. Range: $range');
node.text.removeAttribution(
attribution,
range,

// During style phase, we check which components changed, so we only re-style those components.
// If we just remove the attribution on the same instance of AttributedText, we can't detect the change,
// because comparing the same instance will always evaluate to true.
final spans = node.text.spans.copy();
spans.removeAttribution(
attributionToRemove: attribution,
start: range.start,
end: range.end,
);
node.text = AttributedText(
text: node.text.text,
spans: spans,
);
}
}
Expand Down Expand Up @@ -1073,9 +1093,19 @@ class ToggleTextAttributionsCommand implements EditorCommand {
final node = entry.key;
final range = entry.value;
editorDocLog.info(' - toggling attribution: $attribution. Range: $range');
node.text.toggleAttribution(
attribution,
range,

// During style phase, we check which components changed, so we only re-style those components.
// If we just toggle the attribution on the same instance of AttributedText, we can't detect the change,
// because comparing the same instance will always evaluate to true.
final spans = node.text.spans.copy();
spans.toggleAttribution(
attribution: attribution,
start: range.start,
end: range.end,
);
node.text = AttributedText(
text: node.text.text,
spans: spans,
);
}
}
Expand Down
Loading