Conversation
… instead of only executing one command
…`CommandService` and `ShortcutService`
Contributor
Reviewer's Guide by SourceryThis pull request introduces a new system for registering editor commands with shortcuts, including context-aware shortcuts and weighted command execution. It also adds list-related commands with default shortcuts for improved list manipulation. The changes involve refactoring the command registration process, introducing new command implementations, and updating the shortcut handling mechanism. Sequence diagram for command execution with shortcutsequenceDiagram
participant KeyboardService
participant ShortcutService
participant ShortcutRegistrant
participant CommandService
participant Command
KeyboardService->>ShortcutService: onKeydown(event)
ShortcutService->>ShortcutRegistrant: findShortcut(shortcut)
alt Valid shortcut found
ShortcutRegistrant-->>ShortcutService: Return candidate shortcuts
loop For each candidate shortcut
ShortcutService->>ContextService: contextMatchExpr(candidate.when)
alt Context matches
ShortcutService->>CommandService: executeCommand(commandID, ...args)
CommandService->>Command: run(provider, ...args)
Command-->>CommandService: Return result
alt Result is true
CommandService-->>ShortcutService: Break loop
end
end
end
else No valid shortcut
ShortcutRegistrant-->>ShortcutService: Return empty list
end
ShortcutService-->>KeyboardService: (No command executed)
Updated class diagram for EditorCommandExtensionclassDiagram
class EditorCommandExtension {
- _commandSet: Set<string>
- _commandKeybinding: Map<number, string>
+ registerCommand(command: Command): void
+ registerBasicEditorCommands(extension: IEditorCommandExtension, logService: ILogService, getArguments: () => EditorCommandArguments): void
}
class Command {
+ id: string
+ run(provider: IServiceProvider, editor: IEditorWidget, state: ProseEditorState, dispatch?: (tr: ProseTransaction) => void, view?: ProseEditorView): boolean | Promise<boolean>
}
EditorCommandExtension -- Command : Registers
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Bistard
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey @Bistard - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider renaming
__registerOtherCommandstoregisterBasicCommandsfor clarity and consistency. - The
registerCommandfunction inEditorCommandExtensionshould not directly handle shortcut registration; this logic should be moved to theShortcutRegistrant.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+75
to
+154
| export function splitListItem<TType extends ProseNodeType>(schema: ICommandSchema, listItemType: TType, itemAttrs?: ProseAttrs): Command { | ||
| return new class extends EditorCommandBase { | ||
| public override run(provider: IServiceProvider, editor: IEditorWidget, state: ProseEditorState, dispatch?: (tr: ProseTransaction) => void, view?: ProseEditorView): boolean | Promise<boolean> { | ||
| const { $from, $to, node } = state.selection as ProseNodeSelection; | ||
| if ((node && node.isBlock) || $from.depth < 2 || !$from.sameParent($to)) { | ||
| return false; | ||
| } | ||
|
|
||
| const grandParent = $from.node(-1); | ||
| if (grandParent.type !== listItemType) { | ||
| return false; | ||
| } | ||
|
|
||
| if ($from.parent.content.size === 0 && $from.node(-1).childCount === $from.indexAfter(-1)) { | ||
| // In an empty block. If this is a nested list, the wrapping | ||
| // list item should be split. Otherwise, bail out and let next | ||
| // command handle lifting. | ||
| if ($from.depth === 3 || $from.node(-3).type !== listItemType || | ||
| $from.index(-2) !== $from.node(-2).childCount - 1 | ||
| ) { | ||
| return false; | ||
| } | ||
|
|
||
| if (dispatch) { | ||
| let wrap = ProseFragment.empty; | ||
| const depthBefore = $from.index(-1) ? 1 : $from.index(-2) ? 2 : 3; | ||
|
|
||
| // Build a fragment containing empty versions of the structure | ||
| // from the outer list item to the parent node of the cursor | ||
| for (let d = $from.depth - depthBefore; d >= $from.depth - 3; d--) | ||
| wrap = ProseFragment.from($from.node(d).copy(wrap)); | ||
|
|
||
| const depthAfter = $from.indexAfter(-1) < $from.node(-2).childCount | ||
| ? 1 | ||
| : $from.indexAfter(-2) < $from.node(-3).childCount | ||
| ? 2 | ||
| : 3; | ||
|
|
||
| // Add a second list item with an empty default start node | ||
| wrap = wrap.append(ProseFragment.from(listItemType.createAndFill())); | ||
| const start = $from.before($from.depth - (depthBefore - 1)); | ||
| const tr = state.tr.replace(start, $from.after(-depthAfter), new ProseSlice(wrap, 4 - depthBefore, 0)); | ||
| let sel = -1; | ||
| tr.doc.nodesBetween(start, tr.doc.content.size, (node, pos) => { | ||
| if (sel > -1) { | ||
| return false; | ||
| } | ||
| if (node.isTextblock && node.content.size === 0) { | ||
| sel = pos + 1; | ||
| } | ||
| }); | ||
| if (sel > -1) { | ||
| tr.setSelection(ProseSelection.near(tr.doc.resolve(sel))); | ||
| } | ||
| dispatch(tr.scrollIntoView()); | ||
| } | ||
| return true; | ||
| } | ||
| const nextType = $to.pos === $from.end() | ||
| ? grandParent.contentMatchAt(0).defaultType | ||
| : null; | ||
| const tr = state.tr.delete($from.pos, $to.pos); | ||
| const types = nextType | ||
| ? [itemAttrs | ||
| ? { type: listItemType, attrs: itemAttrs } | ||
| : null, | ||
| { type: nextType }] | ||
| : undefined; | ||
|
|
||
| if (!canSplit(tr.doc, $from.pos, 2, types)) { | ||
| return false; | ||
| } | ||
|
|
||
| if (dispatch) { | ||
| dispatch(tr.split($from.pos, 2, types).scrollIntoView()); | ||
| } | ||
| return true; | ||
| } | ||
| }(schema); | ||
| } |
Contributor
There was a problem hiding this comment.
issue (code-quality): Avoid function declarations, favouring function assignment expressions, inside blocks. (avoid-function-declarations-in-blocks)
Explanation
Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers. Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you should use function expressions, which create functions in-scope.
Comment on lines
+156
to
+251
| export function liftListItem<TType extends ProseNodeType>(schema: ICommandSchema, listItemType: TType): Command { | ||
| return new class extends EditorCommandBase { | ||
| public override run(provider: IServiceProvider, editor: IEditorWidget, state: ProseEditorState, dispatch?: (tr: ProseTransaction) => void, view?: ProseEditorView): boolean | Promise<boolean> { | ||
| const { $from, $to } = state.selection; | ||
| const range = $from.blockRange($to, node => node.childCount > 0 && node.firstChild!.type === listItemType); | ||
| if (!range) { | ||
| return false; | ||
| } | ||
| if (!dispatch) { | ||
| return true; | ||
| } | ||
|
|
||
| if ($from.node(range.depth - 1).type === listItemType) { | ||
| // Inside a parent list | ||
| return this.__liftToOuterList(state, dispatch, listItemType, range); | ||
| } else { | ||
| // Outer list node | ||
| return this.__liftOutOfList(state, dispatch, range); | ||
| } | ||
| } | ||
|
|
||
| private __liftToOuterList(state: ProseEditorState, dispatch: (tr: ProseTransaction) => void, listItemType: ProseNodeType, range: ProseNodeRange) { | ||
| const tr = state.tr, end = range.end, endOfList = range.$to.end(range.depth); | ||
| if (end < endOfList) { | ||
| // There are siblings after the lifted items, which must become | ||
| // children of the last item | ||
| tr.step(new ProseReplaceAroundStep(end - 1, endOfList, end, endOfList, | ||
| new ProseSlice(ProseFragment.from(listItemType.create(null, range.parent.copy())), 1, 0), 1, true)); | ||
| range = new ProseNodeRange(tr.doc.resolve(range.$from.pos), tr.doc.resolve(endOfList), range.depth); | ||
| } | ||
|
|
||
| const target = liftTarget(range); | ||
| if (target === null) { | ||
| return false; | ||
| } | ||
|
|
||
| tr.lift(range, target); | ||
| const $after = tr.doc.resolve(tr.mapping.map(end, -1) - 1); | ||
| if (canJoin(tr.doc, $after.pos) && $after.nodeBefore!.type === $after.nodeAfter!.type) { | ||
| tr.join($after.pos); | ||
| } | ||
|
|
||
| dispatch(tr.scrollIntoView()); | ||
| return true; | ||
| } | ||
|
|
||
| private __liftOutOfList(state: ProseEditorState, dispatch: (tr: ProseTransaction) => void, range: ProseNodeRange) { | ||
| const tr = state.tr, list = range.parent; | ||
|
|
||
| // Merge the list items into a single big item | ||
| for (let pos = range.end, i = range.endIndex - 1, e = range.startIndex; i > e; i--) { | ||
| pos -= list.child(i).nodeSize; | ||
| tr.delete(pos - 1, pos + 1); | ||
| } | ||
|
|
||
| const $start = tr.doc.resolve(range.start), item = $start.nodeAfter!; | ||
| if (tr.mapping.map(range.end) !== range.start + $start.nodeAfter!.nodeSize) { | ||
| return false; | ||
| } | ||
|
|
||
| const atStart = range.startIndex === 0, atEnd = range.endIndex === list.childCount; | ||
| const parent = $start.node(-1), indexBefore = $start.index(-1); | ||
| if (!parent.canReplace(indexBefore + (atStart ? 0 : 1), indexBefore + 1, | ||
| item.content.append(atEnd ? ProseFragment.empty : ProseFragment.from(list))) | ||
| ) { | ||
| return false; | ||
| } | ||
|
|
||
| const start = $start.pos, end = start + item.nodeSize; | ||
| // Strip off the surrounding list. At the sides where we're not at | ||
| // the end of the list, the existing list is closed. At sides where | ||
| // this is the end, it is overwritten to its end. | ||
| tr.step( | ||
| new ProseReplaceAroundStep( | ||
| start - (atStart ? 1 : 0), | ||
| end + (atEnd ? 1 : 0), | ||
| start + 1, | ||
| end - 1, | ||
| new ProseSlice( | ||
| (atStart | ||
| ? ProseFragment.empty | ||
| : ProseFragment.from(list.copy(ProseFragment.empty))).append( | ||
| atEnd | ||
| ? ProseFragment.empty | ||
| : ProseFragment.from(list.copy(ProseFragment.empty)) | ||
| ), | ||
| atStart ? 0 : 1, | ||
| atEnd ? 0 : 1 | ||
| ), | ||
| atStart ? 0 : 1 | ||
| )); | ||
| dispatch(tr.scrollIntoView()); | ||
| return true; | ||
| } | ||
| }(schema); | ||
| } |
Contributor
There was a problem hiding this comment.
issue (code-quality): Avoid function declarations, favouring function assignment expressions, inside blocks. (avoid-function-declarations-in-blocks)
Explanation
Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers. Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you should use function expressions, which create functions in-scope.
Comment on lines
+253
to
+300
| export function sinkListItem<TType extends ProseNodeType>(schema: ICommandSchema, listItemType: TType): Command { | ||
| return new class extends EditorCommandBase { | ||
| public override run(provider: IServiceProvider, editor: IEditorWidget, state: ProseEditorState, dispatch?: (tr: ProseTransaction) => void, view?: ProseEditorView): boolean | Promise<boolean> { | ||
| const { $from, $to } = state.selection; | ||
| const range = $from.blockRange($to, node => node.childCount > 0 && node.firstChild!.type === listItemType); | ||
| if (!range) { | ||
| return false; | ||
| } | ||
| const startIndex = range.startIndex; | ||
| if (startIndex === 0) { | ||
| return false; | ||
| } | ||
| const parent = range.parent, nodeBefore = parent.child(startIndex - 1); | ||
| if (nodeBefore.type !== listItemType) { | ||
| return false; | ||
| } | ||
|
|
||
| if (dispatch) { | ||
| const nestedBefore = nodeBefore.lastChild && nodeBefore.lastChild.type === parent.type; | ||
| const inner = ProseFragment.from(nestedBefore ? listItemType.create() : null); | ||
| const slice = new ProseSlice( | ||
| ProseFragment.from( | ||
| listItemType.create( | ||
| null, | ||
| ProseFragment.from(parent.type.create(null, inner)) | ||
| ) | ||
| ), | ||
| nestedBefore ? 3 : 1, | ||
| 0, | ||
| ); | ||
| const before = range.start, after = range.end; | ||
| dispatch(state.tr.step( | ||
| new ProseReplaceAroundStep( | ||
| before - (nestedBefore ? 3 : 1), | ||
| after, | ||
| before, | ||
| after, | ||
| slice, | ||
| 1, | ||
| true, | ||
| )) | ||
| .scrollIntoView()); | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
| }(schema); | ||
| } |
Contributor
There was a problem hiding this comment.
issue (code-quality): Avoid function declarations, favouring function assignment expressions, inside blocks. (avoid-function-declarations-in-blocks)
Explanation
Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers. Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you should use function expressions, which create functions in-scope.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces list-related operations to the editor, providing commands and shortcuts for splitting, sinking, and lifting list items. It also refactors the command registration process to support shortcut options.
New Features: