singerdmx
fix(macos): Implement actions for ExpandSelectionToDocumentBoundaryIntent and ExpandSelectionToLineBreakIntent to use keyboard shortcuts, unrelated cleanup to the bug fix.
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 #1192
- Related #1937
- Related #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.
- [x] 🛠️ 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.
@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.
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.
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.
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.
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.
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.
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.
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.