From ae82a159fda032ef24a6b5def53b0ee5d1ed7cc5 Mon Sep 17 00:00:00 2001 From: PathGao Date: Wed, 23 Sep 2026 05:45:26 +0800 Subject: [PATCH] fix(editor): give Mod+Shift+K back to Delete Line #652 removed Monaco's Delete Line key because the old Mod+K Shift+R (Delete Row) chord sat one slip away from it. That chord and the whole Mod+K namespace are gone, so the removal guarded nothing, and users coming from VS Code expect the key (#821). Restored on every platform: Cmd+Shift+K on macOS, Ctrl+Shift+K elsewhere. The test now asserts the removal rule stays out; chord ownership is still covered by monacoChordOwnership.spec.ts. --- scripts/formatShortcutKeymap.test.ts | 96 ++++------------------------ scripts/shortcutRegistry.test.ts | 5 +- src/lib/components/Editor.svelte | 27 -------- 3 files changed, 15 insertions(+), 113 deletions(-) diff --git a/scripts/formatShortcutKeymap.test.ts b/scripts/formatShortcutKeymap.test.ts index 13f4124b..7c882946 100644 --- a/scripts/formatShortcutKeymap.test.ts +++ b/scripts/formatShortcutKeymap.test.ts @@ -1,15 +1,10 @@ import assert from 'node:assert/strict'; import test from 'node:test'; -import { KeyCode } from 'monaco-editor/esm/vs/editor/common/standalone/standaloneEnums.js'; -import { KeyMod } from 'monaco-editor/esm/vs/editor/common/services/editorBaseApi.js'; - import { getEditorToolbarTools } from '../src/lib/utils/editorToolbar.js'; import { OperatingSystem, PLATFORMS, - bareCommands, - chordOf, documentKeymap, editorKeymap, registeredActions, @@ -17,7 +12,7 @@ import { type Chord, } from './keymapHarness.js'; import type { ViewerCommand } from '../src/lib/utils/viewerKeymap.js'; -import { readRustBackend, readSource, sliceBetween, sliceFrom } from './sourceTree.js'; +import { readRustBackend, readSource } from './sourceTree.js'; /* * Issues #121 (six formatting commands with no shortcut) and #392 (Ctrl+T means @@ -269,84 +264,19 @@ test('a chord that both layers answer means the same thing in both', () => { * so the check is against ALL of Monaco's chords rather than a remembered * eleven — including Quote's deliberate `inPlaceReplace.down` override, which * is now argued in that file's allow-list instead of asserted by absence here. - * - * What is left in this file is the other direction, below: a Monaco default the - * app takes OFF the keymap rather than takes over. Nothing of ours claims that - * key, so it is not a chord-ownership question and does not belong in the - * ownership file; both ends of it are read out of the installed Monaco. */ -// ------------------------------------- a core Monaco binding, taken back off - -/** - * The `-`-prefixed rules `Editor.svelte` hands `monaco.editor.addKeybindingRules`, - * evaluated with Monaco's own `KeyMod`/`KeyCode`. - * - * The same trick as the rest of this harness: the numbers come from the real - * enums, so a rule that names the wrong chord cannot look right here. - */ -function removedKeybindings(): Array<{ keybinding: number; command: string }> { - const marker = 'monaco.editor.addKeybindingRules('; - const literal = sliceBetween(readSource('src/lib/components/Editor.svelte'), marker, ');').slice(marker.length); - return new Function('monaco', `return ${literal};`)({ KeyMod, KeyCode }); -} - -test('Mod+Shift+K deletes no line here, and Delete Line keeps its palette entry', () => { - // WHY THE KEY GOES. `editor.action.deleteLines` is VS Code's convention, - // inherited by standalone Monaco and inherited again by this app, where it - // silently destroys the line the caret is on. None of the mainstream Markdown - // editors — Typora, Obsidian, iA Writer, Bear — binds it: it is a code-editor - // key that leaked into a prose editor, and it was being mis-fired. - // - // WHY IT IS A REMOVAL RULE. Binding the chord to a no-op `addCommand` would - // make the key dead AND leave a fake command sitting in front of Monaco's; - // a rule whose command id starts with `-` is what Monaco itself reads as - // "drop this default", which is the shape a core binding has to be undone in. - - // 1. That mechanism, read out of the installed Monaco rather than assumed. - const resolver = readSource( - new URL('../node_modules/monaco-editor/esm/vs/platform/keybinding/common/keybindingResolver.js', import.meta.url), - ); - assert.match( - sliceFrom(resolver, 'static handleRemovals('), - /rule\.command\.charAt\(0\) === '-'/, - "Monaco no longer drops defaults by a '-'-prefixed command id", - ); - - // 2. The default being removed, likewise: both the id and the chord come from - // Monaco's source, so a rename or a re-chord upstream fails here instead of - // leaving behind a rule that quietly matches nothing. - const linesOperations = readSource( - new URL('../node_modules/monaco-editor/esm/vs/editor/contrib/linesOperations/browser/linesOperations.js', import.meta.url), - ); - assert.match( - sliceBetween(linesOperations, "id: 'editor.action.deleteLines'", 'run(_accessor'), - /primary: 2048 \/\* KeyMod\.CtrlCmd \*\/ \| 1024 \/\* KeyMod\.Shift \*\/ \| 41 \/\* KeyCode\.KeyK \*\//, - 'Monaco binds Delete Line to some other chord now', +// ------------------------------------- a core Monaco binding, left alone + +test('Mod+Shift+K deletes a line, as it does in Monaco', () => { + // #652 took this key off as a guard against `Mod+K Shift+R` (Delete Row), + // one slip away. That chord and the whole `Mod+K` namespace are gone, so the + // guard protected nothing and cost users a key they asked for back (#821). + // Nothing of ours may claim the chord either; `monacoChordOwnership.spec.ts` + // holds that side. + assert.doesNotMatch( + readSource('src/lib/components/Editor.svelte'), + /-editor\.action\.deleteLines/, + 'Editor.svelte removes Monaco\'s Delete Line key again', ); - - // 3. The rule the app registers, and nothing else: this is a keymap subtraction - // with one entry, and a second one arriving unremarked is a decision nobody - // wrote down. - assert.deepEqual(removedKeybindings(), [ - { keybinding: KeyMod.CtrlCmd | KeyMod.Shift | KeyCode.KeyK, command: '-editor.action.deleteLines' }, - ]); - - // 4. And nothing of the app's took the chord instead. The command stays in the - // command palette — dropping a KEY is not dropping a command, the same rule - // the table delete verbs live under — but the keystroke now does nothing. - for (const platform of PLATFORMS) { - const chord = platform.mac ? 'Shift+Meta+K' : 'Ctrl+Shift+K'; - assert.equal(chordOf(removedKeybindings()[0].keybinding, platform.os), chord); - for (const [id, chords] of editorKeymap(platform.mac, platform.os)) { - assert.ok(!chords.includes(chord), `${platform.name}: ${id} binds ${chord}, the chord just unbound`); - } - for (const command of bareCommands()) { - assert.notEqual( - chordOf(command.binding, platform.os), - chord, - `${platform.name}: ${command.handler} binds ${chord}, the chord just unbound`, - ); - } - } }); diff --git a/scripts/shortcutRegistry.test.ts b/scripts/shortcutRegistry.test.ts index e063dd11..3ab926c0 100644 --- a/scripts/shortcutRegistry.test.ts +++ b/scripts/shortcutRegistry.test.ts @@ -737,9 +737,8 @@ test('Mod+K belongs to Insert Link, on every platform', () => { * than sitting here next to the one the code follows: Delete Column destroys * more than Delete Row does and has a chord (`Mod+Shift+Backspace`), so the old * rationale could not have been what was being applied. The hazard it named is - * also gone in fact: `Mod+Shift+K` is unbound on this branch and the `Mod+K` - * namespace no longer exists, so the near-neighbour that started all this is not - * there to slip onto. + * also gone in fact: the `Mod+K` namespace no longer exists, so no table verb + * sits one slip from Monaco's delete-line (`Mod+Shift+K`) any more. * * The four verbs, and what each one is worth a key for: * diff --git a/src/lib/components/Editor.svelte b/src/lib/components/Editor.svelte index a4c5d2eb..cf8ee1cd 100644 --- a/src/lib/components/Editor.svelte +++ b/src/lib/components/Editor.svelte @@ -851,28 +851,6 @@ EDITING_KEY_CONTEXT, ); - // ⌘⇧K / Ctrl+Shift+K, TAKEN BACK. It is `editor.action.deleteLines`, VS - // Code's convention inherited by standalone Monaco and inherited again by - // this app — a chord that silently destroys the line the caret is on. This - // is a Markdown editor: Typora, Obsidian, iA Writer and Bear all leave that - // chord alone, and it was being mis-fired here. - // - // A REMOVAL RULE, not a no-op command on the same chord. A `-`-prefixed - // command id is what Monaco's own resolver reads as "drop this default" - // (`KeybindingResolver.handleRemovals`); binding a do-nothing command - // instead would leave the key dead AND leave a fake command sitting in - // front of Monaco's, which is worse than either. - // - // The COMMAND stays: an action with no key is still a command palette - // entry (`Mod+P`), which is where the table delete verbs live for exactly - // the same reason. The key goes, the verb does not. - const removedKeybindings = monaco.editor.addKeybindingRules([ - { - keybinding: monaco.KeyMod.CtrlCmd | monaco.KeyMod.Shift | monaco.KeyCode.KeyK, - command: "-editor.action.deleteLines", - }, - ]); - editorReady = true; // After the view-state / anchor-line restore above, deliberately: an @@ -897,11 +875,6 @@ semanticTokens.dispose(); documentSymbols.dispose(); foldingRanges.dispose(); - // The keybinding rules are global to the Monaco module, not to this - // editor, so they are disposed with it rather than left to pile up one - // copy per mount — this component is rebuilt every time a tab goes to - // reading mode and back. - removedKeybindings.dispose(); if (editor && currentTabId) writeEditorPosition(currentTabId);