From d82b989791862d11e1259ec90174ca681b53e7ab Mon Sep 17 00:00:00 2001 From: Jei-sKappa Date: Thu, 21 Mar 2024 13:34:32 +0100 Subject: [PATCH 1/6] fix: incorrect cursor movement behaviour Previously the `moveVertical` function only moved the cursor 'node to node' without thaking into consideration multiline nodes or some portion of a line with different fontSize (ex: a paragraph with size 16 but with a bolded word with size 32). This was caused by the PR #657 that closes #656 and #589. --- lib/src/extensions/position_extension.dart | 150 +++++++++++++++------ 1 file changed, 111 insertions(+), 39 deletions(-) diff --git a/lib/src/extensions/position_extension.dart b/lib/src/extensions/position_extension.dart index 826541514..12b9b08a7 100644 --- a/lib/src/extensions/position_extension.dart +++ b/lib/src/extensions/position_extension.dart @@ -1,3 +1,5 @@ +import 'dart:math' as math; + import 'package:appflowy_editor/appflowy_editor.dart'; import 'package:flutter/material.dart'; @@ -67,66 +69,136 @@ extension PositionExtension on Position { EditorState editorState, { bool upwards = true, }) { + //* GET THE CURRENT OFFSET final selection = editorState.selection; final rects = editorState.selectionRects(); if (rects.isEmpty || selection == null) { return null; } - Offset offset; + Offset currentOffset; + final Rect caretRect; if (selection.isBackward) { - final rect = rects.reduce( - (current, next) => current.bottom >= next.bottom ? current : next, + caretRect = rects.reduce( + (current, next) => current.bottom > next.bottom ? current : next, ); - offset = upwards - ? rect.topRight.translate(0, -rect.height) - : rect.centerRight.translate(0, rect.height); + currentOffset = + upwards ? throw Exception('Ivalid state') : caretRect.bottomRight; } else { - final rect = rects.reduce( + caretRect = rects.reduce( (current, next) => current.top <= next.top ? current : next, ); - offset = upwards - ? rect.topLeft.translate(0, -rect.height) - : rect.centerLeft.translate(0, rect.height); + currentOffset = upwards ? caretRect.topLeft : caretRect.bottomLeft; } - final position = - editorState.service.selectionService.getPositionInOffset(offset); + //* GET THE CURRENT NODE'S TEXT HEIGHT + final node = editorState.document.nodeAtPath(path); + if (node == null) { + return this; + } - if (position != null && !position.path.equals(path)) { - return position; + final nodeRenderBox = node.renderBox; + if (nodeRenderBox == null) { + return this; } - if (upwards) { - final previous = selection.start.path.previous; - if (previous.isNotEmpty && !previous.equals(selection.start.path)) { - final node = editorState.document.nodeAtPath(previous); - final selectable = node?.selectable; - var offset = selection.startIndex; - if (selectable != null) { - offset = offset.clamp( - selectable.start().offset, - selectable.end().offset, - ); - return Position(path: previous, offset: offset); - } + final selectable = node.selectable; + if (selectable == null) { + return this; + } + + final paddingCalculator = editorState.service.rendererService + .blockComponentBuilder(node.type) + ?.configuration + .padding; + + if (paddingCalculator == null) { + // This should not happen. + return this; + } + + final padding = paddingCalculator(node); + final verticalPadding = padding.vertical; + + final rect = selectable.getBlockRect(); + final nodeHeight = rect.height; + final textHeight = nodeHeight - verticalPadding; + final caretHeight = caretRect.height; + final maxYToSkip = upwards ? padding.top : padding.bottom; + + // If the current node is not multiline, this will be ~= 0 + // so the step 1 will be skipped. + final remainingMultilineHeight = (textHeight - caretHeight); + + //* GET THE CLOSEST POSITION TO THE CURRENT OFFSET + //* Step 1: Loop through the current node's text height until the text + //* height is reached or a new position is found. + Offset newOffset = currentOffset; + Position? newPosition; + double minFontSize = + 1; // Consider augmenting this value to increase performance. + double y = minFontSize; + for (; y < remainingMultilineHeight + minFontSize; y += minFontSize) { + newOffset = currentOffset.translate(0, upwards ? -y : y); + + newPosition = + editorState.service.selectionService.getPositionInOffset(newOffset); + + if (newPosition != null && newPosition != this) { + return newPosition; } + } + + //* Step 2: If arrived here it surely hasn't found a different vertical position in the same node (not multiline or last line of a multiline). + //* So we can skip to `maxYToSkip` steps to surely move to a new node. + // We have to decrease 'y' by 'minFontSize' because the last iteration has increased it by 'minFontSize'. + y -= minFontSize; + + // Increase y by the padding slice to skip. + y += maxYToSkip; + newOffset = currentOffset.translate(0, upwards ? -y : y); + + // Determine node's global position. + final nodeYOffsetTop = nodeRenderBox.localToGlobal(Offset.zero).dy; + final nodeYOffsetBottom = nodeYOffsetTop + nodeHeight; + + newOffset = Offset(newOffset.dx, math.min(newOffset.dy, nodeYOffsetBottom)); + + newPosition = + editorState.service.selectionService.getPositionInOffset(newOffset); + + if (newPosition != null && newPosition != this) { + return newPosition; + } + + //* Step 3: If arrived here the previous/next node is not visibile on the screen + //* so we have to manually move to the previous/next node's position. + final List neighbourPath; + final List nodePath; + int offset; + if (upwards) { + neighbourPath = selection.start.path.previous; + nodePath = selection.start.path; + offset = selection.startIndex; } else { - final next = selection.end.path.next; - if (next.isNotEmpty && !next.equals(selection.end.path)) { - final node = editorState.document.nodeAtPath(next); - final selectable = node?.selectable; - var offset = selection.endIndex; - if (selectable != null) { - offset = offset.clamp( - selectable.start().offset, - selectable.end().offset, - ); - return Position(path: next, offset: offset); - } + neighbourPath = selection.end.path.next; + nodePath = selection.end.path; + offset = selection.endIndex; + } + + if (neighbourPath.isNotEmpty && !neighbourPath.equals(nodePath)) { + final neighbour = editorState.document.nodeAtPath(neighbourPath); + final selectable = neighbour?.selectable; + if (selectable != null) { + offset = offset.clamp( + selectable.start().offset, + selectable.end().offset, + ); + return Position(path: neighbourPath, offset: offset); } } + // The cursor is already at the top or bottom of the document. return this; } } From 4245cfa20e8b96e21d781d748b2fb94526795df8 Mon Sep 17 00:00:00 2001 From: Jei-sKappa Date: Thu, 21 Mar 2024 14:44:49 +0100 Subject: [PATCH 2/6] refactor: code refactor and explanatory comments --- lib/src/extensions/position_extension.dart | 141 +++++++++++---------- 1 file changed, 77 insertions(+), 64 deletions(-) diff --git a/lib/src/extensions/position_extension.dart b/lib/src/extensions/position_extension.dart index 12b9b08a7..82b5a6fcf 100644 --- a/lib/src/extensions/position_extension.dart +++ b/lib/src/extensions/position_extension.dart @@ -69,100 +69,112 @@ extension PositionExtension on Position { EditorState editorState, { bool upwards = true, }) { - //* GET THE CURRENT OFFSET - final selection = editorState.selection; + final node = editorState.document.nodeAtPath(path); + if (node == null) { + return this; + } + + final nodeRenderBox = node.renderBox; + if (nodeRenderBox == null) { + return this; + } + + final nodeSelectable = node.selectable; + if (nodeSelectable == null) { + return this; + } + + final editorSelection = editorState.selection; final rects = editorState.selectionRects(); - if (rects.isEmpty || selection == null) { + if (rects.isEmpty || editorSelection == null) { return null; } - Offset currentOffset; + // The offset of outermost part of the caret. + // Either the top if moving upwards, or the bottom if moving downwards. + final Offset caretOffset; + final Rect caretRect; - if (selection.isBackward) { + if (editorSelection.isBackward) { caretRect = rects.reduce( (current, next) => current.bottom > next.bottom ? current : next, ); - currentOffset = - upwards ? throw Exception('Ivalid state') : caretRect.bottomRight; + // No need to check `upwards` because `editorSelection.isBackward` + // implies `upwards` is `false`. + caretOffset = caretRect.bottomRight; } else { caretRect = rects.reduce( (current, next) => current.top <= next.top ? current : next, ); - currentOffset = upwards ? caretRect.topLeft : caretRect.bottomLeft; - } - - //* GET THE CURRENT NODE'S TEXT HEIGHT - final node = editorState.document.nodeAtPath(path); - if (node == null) { - return this; + caretOffset = upwards ? caretRect.topLeft : caretRect.bottomLeft; } - final nodeRenderBox = node.renderBox; - if (nodeRenderBox == null) { - return this; - } - - final selectable = node.selectable; - if (selectable == null) { - return this; - } - - final paddingCalculator = editorState.service.rendererService + final nodeConfig = editorState.service.rendererService .blockComponentBuilder(node.type) - ?.configuration - .padding; - - if (paddingCalculator == null) { + ?.configuration; + if (nodeConfig == null) { // This should not happen. return this; } - final padding = paddingCalculator(node); - final verticalPadding = padding.vertical; - - final rect = selectable.getBlockRect(); - final nodeHeight = rect.height; - final textHeight = nodeHeight - verticalPadding; + final padding = nodeConfig.padding(node); + final nodeRect = nodeSelectable.getBlockRect(); + final nodeHeight = nodeRect.height; + final textHeight = nodeHeight - padding.vertical; final caretHeight = caretRect.height; - final maxYToSkip = upwards ? padding.top : padding.bottom; + + // Minimum (acceptable) font size + // Consider augmenting this value to increase performance. + const minFontSize = 1.0; // If the current node is not multiline, this will be ~= 0 - // so the step 1 will be skipped. + // so the loop will be skipped. final remainingMultilineHeight = (textHeight - caretHeight); - //* GET THE CLOSEST POSITION TO THE CURRENT OFFSET - //* Step 1: Loop through the current node's text height until the text - //* height is reached or a new position is found. - Offset newOffset = currentOffset; + // Linearly search for a new position. + // It's acceptable to use a linear search because the starting point is + // the most outer part of the caret, so: + // - If the current node is multine: + // - If the caret is NOT in the first/last line: at the first iteration + // the cycle a new position (of the previous/next multiline's line) + // will be found, practically ignoring the complexity of the cycle. + // - If the caret is in the first/last line: this is the worst case + // scenario, but only if the padding choosen by the user is very + // large. (padding >= (multiline's textHeight - caretHeight) / 3 + // can start to be considered large. Note that in an average bad case + // scenario the position will be found in 10/12 ms instead of 1/2 ms) + // - If the current node is not multiline: the cycle will be completely + // skipped because `remainingMultilineHeight` would be 0. + Offset newOffset = caretOffset; Position? newPosition; - double minFontSize = - 1; // Consider augmenting this value to increase performance. - double y = minFontSize; - for (; y < remainingMultilineHeight + minFontSize; y += minFontSize) { - newOffset = currentOffset.translate(0, upwards ? -y : y); + for (var y = minFontSize; y < remainingMultilineHeight + minFontSize; y += minFontSize) { + newOffset = caretOffset.translate(0, upwards ? -y : y); newPosition = editorState.service.selectionService.getPositionInOffset(newOffset); + // If a position different from the current one is found, return it. if (newPosition != null && newPosition != this) { return newPosition; } } - //* Step 2: If arrived here it surely hasn't found a different vertical position in the same node (not multiline or last line of a multiline). - //* So we can skip to `maxYToSkip` steps to surely move to a new node. - // We have to decrease 'y' by 'minFontSize' because the last iteration has increased it by 'minFontSize'. - y -= minFontSize; + // If a new position has not been found, it means that the current node + // is not multiline (or the caret is in the last line of a multiline and + // the bottom padding is very large). + // In this case, we can manually skip to the previous/next node position + // by translating the new offset by the padding slice to skip. + // Note that the padding slice to skip can exceed the node's bounds. + final maxSkip = upwards ? padding.top : padding.bottom; - // Increase y by the padding slice to skip. - y += maxYToSkip; - newOffset = currentOffset.translate(0, upwards ? -y : y); + // Translate the new offset by the padding slice to skip. + newOffset = newOffset.translate(0, upwards ? -maxSkip : maxSkip); // Determine node's global position. - final nodeYOffsetTop = nodeRenderBox.localToGlobal(Offset.zero).dy; - final nodeYOffsetBottom = nodeYOffsetTop + nodeHeight; + final nodeHeightOffset = nodeRenderBox.localToGlobal(Offset(0, nodeHeight)); - newOffset = Offset(newOffset.dx, math.min(newOffset.dy, nodeYOffsetBottom)); + // Clamp the new offset to the node's bounds. + newOffset = Offset(newOffset.dx, math.min(newOffset.dy, nodeHeightOffset.dy)); newPosition = editorState.service.selectionService.getPositionInOffset(newOffset); @@ -171,19 +183,20 @@ extension PositionExtension on Position { return newPosition; } - //* Step 3: If arrived here the previous/next node is not visibile on the screen - //* so we have to manually move to the previous/next node's position. + // If a new position has not been found, it means that the current node + // is not visible on the screen. It seems happens only if upwards is true (?) + // In this case, we can manually get the previous/next node position. final List neighbourPath; final List nodePath; int offset; if (upwards) { - neighbourPath = selection.start.path.previous; - nodePath = selection.start.path; - offset = selection.startIndex; + neighbourPath = editorSelection.start.path.previous; + nodePath = editorSelection.start.path; + offset = editorSelection.startIndex; } else { - neighbourPath = selection.end.path.next; - nodePath = selection.end.path; - offset = selection.endIndex; + neighbourPath = editorSelection.end.path.next; + nodePath = editorSelection.end.path; + offset = editorSelection.endIndex; } if (neighbourPath.isNotEmpty && !neighbourPath.equals(nodePath)) { From d4ae506fde4cf6f78d54c6b0eac11424e83d5b6f Mon Sep 17 00:00:00 2001 From: Jei-sKappa Date: Thu, 28 Mar 2024 10:45:47 +0100 Subject: [PATCH 3/6] style: code formatting --- lib/src/extensions/position_extension.dart | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/src/extensions/position_extension.dart b/lib/src/extensions/position_extension.dart index 82b5a6fcf..4bc826ded 100644 --- a/lib/src/extensions/position_extension.dart +++ b/lib/src/extensions/position_extension.dart @@ -147,7 +147,9 @@ extension PositionExtension on Position { // skipped because `remainingMultilineHeight` would be 0. Offset newOffset = caretOffset; Position? newPosition; - for (var y = minFontSize; y < remainingMultilineHeight + minFontSize; y += minFontSize) { + for (var y = minFontSize; + y < remainingMultilineHeight + minFontSize; + y += minFontSize) { newOffset = caretOffset.translate(0, upwards ? -y : y); newPosition = @@ -174,7 +176,10 @@ extension PositionExtension on Position { final nodeHeightOffset = nodeRenderBox.localToGlobal(Offset(0, nodeHeight)); // Clamp the new offset to the node's bounds. - newOffset = Offset(newOffset.dx, math.min(newOffset.dy, nodeHeightOffset.dy)); + newOffset = Offset( + newOffset.dx, + math.min(newOffset.dy, nodeHeightOffset.dy), + ); newPosition = editorState.service.selectionService.getPositionInOffset(newOffset); From 7479f8c907991035feede9f5ca9c2b8ac6a7e0cb Mon Sep 17 00:00:00 2001 From: Jei-sKappa Date: Sat, 13 Apr 2024 17:27:49 +0200 Subject: [PATCH 4/6] fix: wrong node path --- lib/src/extensions/position_extension.dart | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/lib/src/extensions/position_extension.dart b/lib/src/extensions/position_extension.dart index 4bc826ded..f9f0cd258 100644 --- a/lib/src/extensions/position_extension.dart +++ b/lib/src/extensions/position_extension.dart @@ -99,9 +99,7 @@ extension PositionExtension on Position { caretRect = rects.reduce( (current, next) => current.bottom > next.bottom ? current : next, ); - // No need to check `upwards` because `editorSelection.isBackward` - // implies `upwards` is `false`. - caretOffset = caretRect.bottomRight; + caretOffset = upwards ? caretRect.topRight : caretRect.bottomRight; } else { caretRect = rects.reduce( (current, next) => current.top <= next.top ? current : next, @@ -191,19 +189,9 @@ extension PositionExtension on Position { // If a new position has not been found, it means that the current node // is not visible on the screen. It seems happens only if upwards is true (?) // In this case, we can manually get the previous/next node position. - final List neighbourPath; - final List nodePath; - int offset; - if (upwards) { - neighbourPath = editorSelection.start.path.previous; - nodePath = editorSelection.start.path; - offset = editorSelection.startIndex; - } else { - neighbourPath = editorSelection.end.path.next; - nodePath = editorSelection.end.path; - offset = editorSelection.endIndex; - } - + int offset = editorSelection.end.offset; + final List nodePath = editorSelection.end.path; + final List neighbourPath = upwards ? nodePath.previous : nodePath.next; if (neighbourPath.isNotEmpty && !neighbourPath.equals(nodePath)) { final neighbour = editorState.document.nodeAtPath(neighbourPath); final selectable = neighbour?.selectable; From 0afd6a98f1d95b438da8bcc2f1c64d4248aae8dd Mon Sep 17 00:00:00 2001 From: Mathias Mogensen Date: Tue, 30 Apr 2024 22:37:49 +0200 Subject: [PATCH 5/6] chore: code review --- lib/src/extensions/position_extension.dart | 37 +++++++++++----------- 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/lib/src/extensions/position_extension.dart b/lib/src/extensions/position_extension.dart index f9f0cd258..b5be51100 100644 --- a/lib/src/extensions/position_extension.dart +++ b/lib/src/extensions/position_extension.dart @@ -1,8 +1,9 @@ import 'dart:math' as math; -import 'package:appflowy_editor/appflowy_editor.dart'; import 'package:flutter/material.dart'; +import 'package:appflowy_editor/appflowy_editor.dart'; + enum SelectionRange { character, word, @@ -90,28 +91,28 @@ extension PositionExtension on Position { return null; } + final Rect caretRect = rects.reduce((current, next) { + if (editorSelection.isBackward) { + return current.bottom > next.bottom ? current : next; + } + return current.top <= next.top ? current : next; + }); + // The offset of outermost part of the caret. // Either the top if moving upwards, or the bottom if moving downwards. - final Offset caretOffset; - - final Rect caretRect; - if (editorSelection.isBackward) { - caretRect = rects.reduce( - (current, next) => current.bottom > next.bottom ? current : next, - ); - caretOffset = upwards ? caretRect.topRight : caretRect.bottomRight; - } else { - caretRect = rects.reduce( - (current, next) => current.top <= next.top ? current : next, - ); - caretOffset = upwards ? caretRect.topLeft : caretRect.bottomLeft; - } + final Offset caretOffset = editorSelection.isBackward + ? upwards + ? caretRect.topRight + : caretRect.bottomRight + : upwards + ? caretRect.topLeft + : caretRect.bottomLeft; final nodeConfig = editorState.service.rendererService .blockComponentBuilder(node.type) ?.configuration; if (nodeConfig == null) { - // This should not happen. + assert(nodeConfig != null, 'Block Configuration should not be null'); return this; } @@ -123,7 +124,7 @@ extension PositionExtension on Position { // Minimum (acceptable) font size // Consider augmenting this value to increase performance. - const minFontSize = 1.0; + const double minFontSize = 1.0; // If the current node is not multiline, this will be ~= 0 // so the loop will be skipped. @@ -145,7 +146,7 @@ extension PositionExtension on Position { // skipped because `remainingMultilineHeight` would be 0. Offset newOffset = caretOffset; Position? newPosition; - for (var y = minFontSize; + for (double y = minFontSize; y < remainingMultilineHeight + minFontSize; y += minFontSize) { newOffset = caretOffset.translate(0, upwards ? -y : y); From ebf0718697bc2dd07f99933c848daff6610cdc73 Mon Sep 17 00:00:00 2001 From: Mathias Mogensen Date: Tue, 30 Apr 2024 22:38:55 +0200 Subject: [PATCH 6/6] chore: final review change --- lib/src/extensions/position_extension.dart | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/lib/src/extensions/position_extension.dart b/lib/src/extensions/position_extension.dart index b5be51100..8c6812d68 100644 --- a/lib/src/extensions/position_extension.dart +++ b/lib/src/extensions/position_extension.dart @@ -71,17 +71,9 @@ extension PositionExtension on Position { bool upwards = true, }) { final node = editorState.document.nodeAtPath(path); - if (node == null) { - return this; - } - - final nodeRenderBox = node.renderBox; - if (nodeRenderBox == null) { - return this; - } - - final nodeSelectable = node.selectable; - if (nodeSelectable == null) { + final nodeRenderBox = node?.renderBox; + final nodeSelectable = node?.selectable; + if (node == null || nodeRenderBox == null || nodeSelectable == null) { return this; }