fix(macos): Implement actions for ExpandSelectionToDocumentBoundaryIntent and ExpandSelectionToLineBreakIntent to use keyboard shortcuts, unrelated cleanup to the bug fix. #2279

EchoEllet · 2024-09-24T17:39:29Z

Description

Implement actions for ExpandSelectionToDocumentBoundaryIntent and ExpandSelectionToLineBreakIntent in addition to unrelated cleanup that is not required to have the bug fix.

This PR will be updated soon, see #1192 for more details about the issue.

Related Issues

Fix [Desktop] Error when using selection shortcut on Mac #1192
Related fix(editor): implement editor shortcut action for home and end keys to fix exception about unimplemented ScrollToDocumentBoundaryIntent #1937
Related Invoking macOS intents that have missing actions #2288

ExtendSelectionToNextParagraphBoundaryOrCaretLocationIntent is also missing and will throw when pressing (option + shift + arrow up/down).

Type of Change

✨ New feature: Adds new functionality without breaking existing features.
🛠️ Bug fix: Resolves an issue without altering current behavior.
🧹 Code refactor: Code restructuring that does not affect behavior.
❌ Breaking change: Alters existing functionality and requires updates.
🧪 Tests: Adds new tests or modifies existing tests.
📝 Documentation: Updates or additions to documentation.
🗑️ Chore: Routine tasks, or maintenance.
✅ Build configuration change: Changes to build or deploy processes.

Suggestions

Not for merge yet.

TODO

Explain the changes
Add tests, improve docs, and clear the Todos in code.

…t and ExpandSelectionToLineBreakIntent

EchoEllet · 2024-09-25T17:46:11Z

@CatHood0 I will address the change of
QuillKeyboardServiceWidget directly in this PR, worrying about having a clean history, separation, and organization of changes is not our first priority at the moment, I keep forgetting about adding this to the Todo and already forgot many things since I delay them later, my TODO is already full.

The support for shortcuts is not clean enough, some of them are overridable:

  HideSelectionToolbarIntent:
        _makeOverridable(QuillEditorHideSelectionToolbarAction(this)),
    UndoTextIntent: _makeOverridable(QuillEditorUndoKeyboardAction(this)),
    RedoTextIntent: _makeOverridable(QuillEditorRedoKeyboardAction(this)),

Some of them do start with QuillEditor prefix and some of them do not:

    ToggleTextStyleIntednt: _formatSelectionAction,
    IndentSelectionIntent: _indentSelectionAction,
    QuillEditorApplyHeaderIntent: _applyHeaderAction,
    QuillEditorApplyCheckListIntent: _applyCheckListAction,
    // TODO: Double check and see if those and related should be overridable
    QuillEditorApplyLinkIntent: QuillEditorApplyLinkAction(this),
    ScrollToDocumentBoundaryIntent: NavigateToDocumentBoundaryAction(this),

Others are not instantiated directly and have private member and the only usage is in the shortcuts:

    ToggleTextStyleIntent: _formatSelectionAction,
    IndentSelectionIntent: _indentSelectionAction,
    QuillEditorApplyHeaderIntent: _applyHeaderAction,
    QuillEditorApplyCheckListIntent: _applyCheckListAction,

It's not clear how should we name things, we need to agree on something, each class is named very differently without conventions or guidelines, it's also not clear that if we should add the SingleActivator (see #1937 comment) or if they are already registered with Flutter, or if we're using something somewhere that do register those, why they those actions are missing in the first place? It's causing issues on the desktop with keyboard shortcuts.

In addition to other questions and issues, I didn't register them in the TODOs.

The file default_single_activator_actions.dart should also be somewhere inside raw_editor.

…efaultSinlgeActivatorIntents() and QuillKeyboardServiceWidget to EditorKeyboardShortcuts, move their files inside raw_editor directory

…torIntents() from editor_keyboard_shortcuts.dart

…_activator_intents.dart to match the name of defaultSinlgeActivatorIntents()

EchoEllet · 2024-09-25T18:39:54Z

It seems that there are even more important things to clean up, I have also noticed some duplicated logic, and a new update will be soon however it's not related to this PR so it might be confusing to review. I have tried to separate it into a new PR though decided not to.

EchoEllet · 2024-09-25T19:00:37Z

or if we're using something somewhere that do register those, why they those actions are missing in the first place

It seems that we're using intentForMacOSSelector() from Flutter inside the raw_editor_state.dart without implementing the actions for each intent.

Details

We need to file an issue for this and fix them all in a clean way so we don't lose track of this.

Reported in #2288.

…utsActions outside of QuillRawEditorState

…or, update macOS example Podfile.lock

…od and add assert to check the platform

EchoEllet · 2024-09-25T20:04:38Z

lib/src/editor/raw_editor/keyboard_shortcuts/default_single_activator_intents.dart

+    // Navigate to the start or end of the document
+    SingleActivator(
+      LogicalKeyboardKey.home,
+      control: !_isDesktopMacOS,
+      meta: _isDesktopMacOS,
+    ): const ScrollToDocumentBoundaryIntent(forward: false),
+    SingleActivator(
+      LogicalKeyboardKey.end,
+      control: !_isDesktopMacOS,
+      meta: _isDesktopMacOS,
+    ): const ScrollToDocumentBoundaryIntent(forward: true),


Those two included in #1937. I wasn't and still not sure if this is needed, seems to be redundant though.

EchoEllet · 2024-09-25T20:06:09Z

lib/src/editor/raw_editor/raw_editor_actions.dart

+    // Plain text of the document (needed to find line breaks)
+    final text = state.controller.plainTextEditingValue.text;
+
+    final currentSelection = state.controller.selection;
+
+    // Calculate the next or previous line break based on direction
+    final searchStartOffset = currentSelection.extentOffset;
+
+    final targetLineBreak = () {
+      if (intent.forward) {
+        final nextLineBreak = text.indexOf('\n', searchStartOffset);
+        final noNextLineBreak = nextLineBreak == -1;
+        return noNextLineBreak ? text.length : nextLineBreak + 1;
+      }
+
+      // Backward
+
+      // Ensure (searchStartOffset - 1) is not negative to avoid [RangeError]
+      final safePreviousSearchOffset =
+          (searchStartOffset > 0) ? (searchStartOffset - 1) : 0;
+
+      final previousLineBreak =
+          text.lastIndexOf('\n', safePreviousSearchOffset);
+
+      final noPreviousLineBreak = previousLineBreak == -1;
+      return noPreviousLineBreak ? 0 : previousLineBreak;
+    }();
+
+    // Create a new selection, extending it to the line break was found
+    final newSelection = currentSelection.copyWith(
+      extentOffset: targetLineBreak,
+    );
+
+    return Actions.invoke(
+      context ?? (throw StateError('BuildContext should not be null.')),
+      UpdateSelectionIntent(
+        state.textEditingValue,
+        newSelection,
+        SelectionChangedCause.keyboard,
+      ),


Will need to know if this functionality already exist as a function to call, prefer to not repeat it twice, it can be confusing for other developers and for me in the future.

Related _linebreak()

AtlasAutocode

The support for shortcuts is not clean enough, some of them are overridable:

Just a comment: I have had a brief encounter with _makeOverridable and was surprised by how it works:

Overridable means it can be overridden by prior implementations. That is exactly the opposite of what I understand by the term.

For example: Widget build Stack:
Widget 1 overridable action
Widget 2 overridable action
Widget 3 uses action from Widget 1! Because the prior implementation takes priority.

In order for the implementation in Widget 2 to correct a defect in Widget 1's implementation, it has to not use _makeOverridable.

I hope I am wrong in this.

In my case, I seem to recollect that there was an error in how Flutter handled a macOS shortcut key and since Flutter was at the top of the rendering stack it always took priority so my fix would never work if I used _makeOverridable!

EchoEllet · 2024-09-25T20:46:19Z

The current code is quite confusing, even after the cleanup we have editor_keyboard_shortcut_actions.dart and raw_editor_actions.dart, ScrollToDocumentBoundaryIntent is already registered intent by default so including it in:

  SingleActivator(
      LogicalKeyboardKey.home,
      control: !_isDesktopMacOS,
      meta: _isDesktopMacOS,
    ): const ScrollToDocumentBoundaryIntent(forward: false),
    SingleActivator(
      LogicalKeyboardKey.end,
      control: !_isDesktopMacOS,
      meta: _isDesktopMacOS,
    ): const ScrollToDocumentBoundaryIntent(forward: true),

Doesn't make any difference, I still want to know if this is macOS specific and why we don't need to register a activator for it before moving further.

Was added in #1937.

…tcutsActionsManager, move raw_editor_actions to keyboard_shortcuts/editor_keyboard_shortcut_actions_manager.dart

…EditorState

EchoEllet · 2024-09-25T21:23:16Z

See #2288, while cleaning up, I'm having even more questions and the answers are not clear, I have spent almost one or two days trying to fix a minor bug, the bug has been fixed without fixing the related code quality issues, I prefer to completely rewrite the Keyboard shortcuts with their actions and intent instead and might do in a separate PR or might improve it later.

Hopefully was able to move around 170 lines of code from QuillRawEditorState.

@CatHood0 @AtlasAutocode If you can review the PR, once addressing all the changes, will merge this PR for now.

CatHood0 · 2024-09-25T23:14:58Z

Sorry I haven't been able to fix a few issues yet. I've had a hard time these days. I'll take the time tonight to do some code reviews and fix issues that were already on my list.

EchoEllet · 2024-09-25T23:19:36Z

No need to be sorry, take as much as time as you need.

I appreciate your commitment to tackling the code reviews and issues tonight. If you need any assistance or just want to discuss anything, feel free to reach out.

EchoEllet · 2024-10-23T16:17:19Z

This PR is not the best in improving the organization and code quality though it's slightly more organized than the current code. I will merge however I have many questions, some things are unclear, and it's less time-consuming to rewrite the shortcuts functionality. This is a bug fix and unrelated and unrelated to the issue.

I'm open to feedback if anything can be improved here.

…tent and ExpandSelectionToLineBreakIntent to use keyboard shortcuts, unrelated cleanup to the bug fix. (singerdmx#2279)

EchoEllet added 2 commits September 24, 2024 20:03

chore: add related TODO

7946beb

fix(keyboard): add actions for ExpandSelectionToDocumentBoundaryInten…

03847cc

…t and ExpandSelectionToLineBreakIntent

EchoEllet changed the title ~~Fix/1192~~ fix(keyboard): Implement actions for ExpandSelectionToDocumentBoundaryIntent and ExpandSelectionToLineBreakIntent. Sep 24, 2024

EchoEllet changed the title ~~fix(keyboard): Implement actions for ExpandSelectionToDocumentBoundaryIntent and ExpandSelectionToLineBreakIntent.~~ fix(macos): Implement actions for ExpandSelectionToDocumentBoundaryIntent and ExpandSelectionToLineBreakIntent to use keyboard shortcuts. Sep 24, 2024

EchoEllet added minor Minimal impact or cosmetic issue. Can be resolved at a later time without affecting overall function macOS Issues or feature requests specific to the macOS platform. labels Sep 24, 2024

feat(macos): implement ExpandSelectionToLineBreakAction invoke() method

e591f89

EchoEllet mentioned this pull request Sep 25, 2024

[Desktop] Error when using selection shortcut on Mac #1192

Closed

EchoEllet requested review from singerdmx, CatHood0 and AtlasAutocode September 25, 2024 17:32

chore: unrelated cleanup, rename defaultSinlgeActivatorActions() to d…

63e30ff

…efaultSinlgeActivatorIntents() and QuillKeyboardServiceWidget to EditorKeyboardShortcuts, move their files inside raw_editor directory

EchoEllet added 5 commits September 25, 2024 21:01

chore: add comma to format shortcuts property to be a bit more readable

95bc244

chore: add comma to format actions property to be a bit more readable

ad1573f

chore: use isMacOS directly without passing it to defaultSinlgeActiva…

b29e39e

…torIntents() from editor_keyboard_shortcuts.dart

chore: remove unused import

9c9eb3a

chore: rename default_single_activator_actions.dart to default_single…

7dfe3c2

…_activator_intents.dart to match the name of defaultSinlgeActivatorIntents()

EchoEllet mentioned this pull request Sep 25, 2024

Invoking macOS intents that have missing actions #2288

Open

EchoEllet added 4 commits September 25, 2024 22:10

refactor: extracrt keyboard shorcut actions into EditorKeyboardShortc…

8ea386e

…utsActions outside of QuillRawEditorState

Merge remote-tracking branch 'origin/master' into fix/1192

da68cd3

fix: incorrect usage of BuildContext in QuillRawEditorState construct…

7cc8941

…or, update macOS example Podfile.lock

chore: explain that performSelector() is macOS platform specific meth…

9d5385b

…od and add assert to check the platform

EchoEllet commented Sep 25, 2024

View reviewed changes

AtlasAutocode reviewed Sep 25, 2024

View reviewed changes

EchoEllet added 2 commits September 25, 2024 23:52

refactor: rename EditorKeyboardShortcutsActions to EditorKeyboardShor…

e299b9c

…tcutsActionsManager, move raw_editor_actions to keyboard_shortcuts/editor_keyboard_shortcut_actions_manager.dart

chore: rename _shortcutActions to _shortcutActionsManager in QuillRaw…

d4486cf

…EditorState

singerdmx approved these changes Sep 25, 2024

View reviewed changes

EchoEllet added 2 commits September 26, 2024 00:14

chore: remove redundant Single activator entries that's not being used

8652cb3

chore: remove the TODOs without properly finishing them

d6bc3c1

EchoEllet added 3 commits October 23, 2024 19:01

Merge branch 'master' into fix/1192

7a3ab5c

chore: remove the TODOs

03c2da9

chore: remove a TODO

7d03682

EchoEllet marked this pull request as ready for review October 23, 2024 16:10

EchoEllet merged commit e7d3391 into master Oct 23, 2024
2 checks passed

EchoEllet deleted the fix/1192 branch October 23, 2024 16:17

EchoEllet added a commit that referenced this pull request Oct 23, 2024

chore: merge origin/master and fix conflicts with #2279

0ec8c88

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(macos): Implement actions for ExpandSelectionToDocumentBoundaryIntent and ExpandSelectionToLineBreakIntent to use keyboard shortcuts, unrelated cleanup to the bug fix. #2279

fix(macos): Implement actions for ExpandSelectionToDocumentBoundaryIntent and ExpandSelectionToLineBreakIntent to use keyboard shortcuts, unrelated cleanup to the bug fix. #2279

EchoEllet commented Sep 24, 2024 •

edited

Loading

EchoEllet commented Sep 25, 2024

EchoEllet commented Sep 25, 2024

EchoEllet commented Sep 25, 2024 •

edited

Loading

EchoEllet Sep 25, 2024 •

edited

Loading

EchoEllet Sep 25, 2024

EchoEllet Sep 25, 2024

AtlasAutocode left a comment

EchoEllet commented Sep 25, 2024 •

edited

Loading

EchoEllet commented Sep 25, 2024

CatHood0 commented Sep 25, 2024 •

edited

Loading

EchoEllet commented Sep 25, 2024

EchoEllet commented Oct 23, 2024

fix(macos): Implement actions for ExpandSelectionToDocumentBoundaryIntent and ExpandSelectionToLineBreakIntent to use keyboard shortcuts, unrelated cleanup to the bug fix. #2279

fix(macos): Implement actions for ExpandSelectionToDocumentBoundaryIntent and ExpandSelectionToLineBreakIntent to use keyboard shortcuts, unrelated cleanup to the bug fix. #2279

Conversation

EchoEllet commented Sep 24, 2024 • edited Loading

Description

Related Issues

Type of Change

Suggestions

TODO

EchoEllet commented Sep 25, 2024

EchoEllet commented Sep 25, 2024

EchoEllet commented Sep 25, 2024 • edited Loading

EchoEllet Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

EchoEllet Sep 25, 2024

Choose a reason for hiding this comment

EchoEllet Sep 25, 2024

Choose a reason for hiding this comment

AtlasAutocode left a comment

Choose a reason for hiding this comment

EchoEllet commented Sep 25, 2024 • edited Loading

EchoEllet commented Sep 25, 2024

CatHood0 commented Sep 25, 2024 • edited Loading

EchoEllet commented Sep 25, 2024

EchoEllet commented Oct 23, 2024

EchoEllet commented Sep 24, 2024 •

edited

Loading

EchoEllet commented Sep 25, 2024 •

edited

Loading

EchoEllet Sep 25, 2024 •

edited

Loading

EchoEllet commented Sep 25, 2024 •

edited

Loading

CatHood0 commented Sep 25, 2024 •

edited

Loading