Add formatting keyboard shortcuts (Ctrl+B bold, Ctrl+I italic, etc.) - #29
Add formatting keyboard shortcuts (Ctrl+B bold, Ctrl+I italic, etc.)#29Wavesonics wants to merge 1 commit into
Conversation
Wire inline formatting shortcuts into the editor's key handler, reusing the same span-toggle code path as the markdown toolbar: - Ctrl+B: bold - Ctrl+I: italic - Ctrl+Shift+X: strikethrough - Ctrl+E: inline code Bindings and their target SpanStyles are configurable via the new TextFormattingShortcuts, exposed as a parameter on TextEditor and BasicTextEditor. Styles default to the markdown defaults so toggled spans round-trip through the markdown extension; pass TextFormattingShortcuts.None to disable. Closes #22
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 28 |
| Duplication | 0 |
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull Request Overview
There is a severe misalignment between this PR's title/description and the actual code changes. While the PR claims to implement formatting keyboard shortcuts (Ctrl+B, Ctrl+I, etc.) and a new TextFormattingShortcuts API, the diff contains only accessibility semantics logic for BasicTextEditor. None of the listed acceptance criteria for keyboard styling shortcuts have been met.
Furthermore, the logic provided for accessibility semantics contains a functional flaw where the relative parameter in selection updates is ignored, and it lacks unit testing to ensure boundary conditions are handled during coordinate mapping. This PR should not be merged in its current state as it fails to deliver the requested feature and introduces incomplete accessibility logic.
About this PR
- Critical Intent Mismatch: The PR description discusses keyboard shortcut dispatching and a new styling API, but the submitted code exclusively implements accessibility semantics for text manipulation. The 'formattingShortcuts' parameter and 'TextFormattingShortcuts' data class are entirely missing.
- The PR description mentions 'FormattingShortcutTest', but no test files were included in the commit. Please ensure all related files are staged and pushed.
Test suggestions
- Missing recommended test scenario: Toggle Bold style using Ctrl+B on a selected range
- Missing recommended test scenario: Toggle Italic style using Ctrl+I on a selected range
- Missing recommended test scenario: Toggle Inline Code style using Ctrl+E on a selected range
- Missing recommended test scenario: Verify Ctrl+Shift+X applies Strikethrough and does not trigger the Cut (Ctrl+X) operation
- Missing recommended test scenario: Set a pending style at the cursor with no selection and verify subsequent typing uses that style
- Missing recommended test scenario: Verify shortcuts are ignored when the editor is disabled
- Verify that setSelection and insertTextAtCursor correctly update state when provided with various string indices
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Missing recommended test scenario: Toggle Bold style using Ctrl+B on a selected range
2. Missing recommended test scenario: Toggle Italic style using Ctrl+I on a selected range
3. Missing recommended test scenario: Toggle Inline Code style using Ctrl+E on a selected range
4. Missing recommended test scenario: Verify Ctrl+Shift+X applies Strikethrough and does not trigger the Cut (Ctrl+X) operation
5. Missing recommended test scenario: Set a pending style at the cursor with no selection and verify subsequent typing uses that style
6. Missing recommended test scenario: Verify shortcuts are ignored when the editor is disabled
7. Verify that setSelection and insertTextAtCursor correctly update state when provided with various string indices
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| .semantics { | ||
| editableText = state.getAllText() | ||
| textSelectionRange = state.selectionAsTextRange() | ||
| setText { newText -> | ||
| state.setText(newText) | ||
| true | ||
| } | ||
| insertTextAtCursor { newText -> | ||
| if (state.selector.hasSelection()) state.selector.deleteSelection() | ||
| state.insertStringAtCursor(newText) | ||
| true | ||
| } | ||
| setSelection { start, end, _ -> | ||
| val length = state.getTextLength() | ||
| val from = start.coerceIn(0, length) | ||
| val to = end.coerceIn(0, length) | ||
| if (from == to) { | ||
| state.cursor.updatePosition(state.getOffsetAtCharacter(from)) | ||
| state.selector.clearSelection() | ||
| } else { | ||
| state.selector.updateSelection( | ||
| state.getOffsetAtCharacter(from), | ||
| state.getOffsetAtCharacter(to), | ||
| ) | ||
| state.cursor.updatePosition(state.getOffsetAtCharacter(to)) | ||
| } | ||
| true | ||
| } | ||
| onClick { focusRequester.requestFocus(); true } | ||
| } |
There was a problem hiding this comment.
🔴 HIGH RISK
The implementation diverges entirely from the PR description. This file adds accessibility semantics (setText, setSelection, etc.) but contains no logic for keyboard shortcuts (Ctrl+B, Ctrl+I, etc.) or the promised formattingShortcuts API. Please verify if the intended logic was omitted from this commit.
| state.insertStringAtCursor(newText) | ||
| true | ||
| } | ||
| setSelection { start, end, _ -> |
There was a problem hiding this comment.
🟡 MEDIUM RISK
The relative parameter in setSelection is currently ignored. You should handle cases where relative is true by adding the provided offsets to the current selection indices.
| setText { newText -> | ||
| state.setText(newText) | ||
| true | ||
| } | ||
| insertTextAtCursor { newText -> | ||
| if (state.selector.hasSelection()) state.selector.deleteSelection() | ||
| state.insertStringAtCursor(newText) | ||
| true | ||
| } | ||
| setSelection { start, end, _ -> | ||
| val length = state.getTextLength() | ||
| val from = start.coerceIn(0, length) | ||
| val to = end.coerceIn(0, length) | ||
| if (from == to) { | ||
| state.cursor.updatePosition(state.getOffsetAtCharacter(from)) | ||
| state.selector.clearSelection() | ||
| } else { | ||
| state.selector.updateSelection( | ||
| state.getOffsetAtCharacter(from), | ||
| state.getOffsetAtCharacter(to), | ||
| ) | ||
| state.cursor.updatePosition(state.getOffsetAtCharacter(to)) | ||
| } | ||
| true | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: The logic for setSelection and insertTextAtCursor involves manual coordinate mapping and state updates. This should be covered by unit tests to ensure indices are correctly coerced and the cursor position remains synchronized with the text state.
| } else { | ||
| state.selector.updateSelection( | ||
| state.getOffsetAtCharacter(from), | ||
| state.getOffsetAtCharacter(to), | ||
| ) | ||
| state.cursor.updatePosition(state.getOffsetAtCharacter(to)) | ||
| } |
There was a problem hiding this comment.
⚪ LOW RISK
Nitpick: In the else block, calculate state.getOffsetAtCharacter(to) once and store it in a variable to be reused for both state.selector.updateSelection and state.cursor.updatePosition to improve readability and efficiency.
There was a problem hiding this comment.
Pull Request Overview
The PR successfully implements the requested keyboard shortcuts and configuration options, meeting most acceptance criteria. However, there is a critical logic gap in TextEditorKeyCommandHandler.kt: if the strikethrough shortcut is disabled via configuration, the Ctrl+Shift+X combination falls through and triggers the 'Cut' handler, which currently only checks for isCtrlPressed. This violates the requirement to prioritize strikethrough over cut and introduces a regression risk for text selection management.
Additionally, the current implementation uses hardcoded Control keys across all platforms. On macOS, this conflicts with standard Emacs-style navigation shortcuts and deviates from the expected use of the Command (Meta) key for formatting. The TextEditorKeyCommandHandler has also reached a high level of cognitive complexity (+16) which warrants refactoring to ensure long-term maintainability.
About this PR
- Systemic Complexity: The addition of multiple conditional formatting shortcuts into the main keyboard event handler has increased its cognitive complexity significantly. This makes the core editor logic harder to maintain and test.
2 comments outside of the diff
ComposeTextEditor/src/commonMain/kotlin/com/darkrockstudios/texteditor/input/TextEditorInputModifierNode.kt
line 145⚪ LOW RISK
Nitpick: Expose theformattingShortcutsproperty in the inspector.Try running the following prompt in your coding agent:
In
TextEditorInputModifierElement, update theinspectablePropertiesmethod to include a property entry forformattingShortcuts.
ComposeTextEditor/src/commonMain/kotlin/com/darkrockstudios/texteditor/input/TextEditorKeyCommandHandler.kt
line 59🟡 MEDIUM RISK
Suggestion: ThehandleKeyEventfunction is becoming quite large and complex. Consider refactoring the shortcut logic by extracting formatting commands into a dedicated handler or a lookup map. This would decouple the core navigation/editing logic from optional formatting features.Try running the following prompt in your IDE agent:
Refactor the
handleKeyEventfunction inTextEditorKeyCommandHandler.ktto move the formatting shortcut logic (Ctrl+B, I, E, Shift+X) into a separate private method calledhandleFormattingShortcuts, reducing the complexity of the mainwhenblock.
Test suggestions
- Ctrl+B toggles bold on selection
- Ctrl+I toggles italic on selection
- Ctrl+Shift+X toggles strikethrough on selection without triggering cut
- Ctrl+E toggles inline code on selection
- Shortcuts apply pending cursor style when no selection is present
- Shortcuts are ignored when the editor is disabled
- Shortcuts can be unbound or disabled via configuration
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| true | ||
| } | ||
|
|
||
| keyEvent.isCtrlPressed && keyEvent.key == Key.X -> { |
There was a problem hiding this comment.
🔴 HIGH RISK
To prevent accidental 'Cut' operations when strikethrough shortcuts are disabled or when other Shift+X combinations are used, ensure the Cut handler explicitly checks that Shift is not pressed.
This might be a simple fix:
| keyEvent.isCtrlPressed && keyEvent.key == Key.X -> { | |
| keyEvent.isCtrlPressed && !keyEvent.isShiftPressed && keyEvent.key == Key.X -> { |
| true | ||
| } | ||
|
|
||
| keyEvent.isCtrlPressed && keyEvent.key == Key.B && |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: Restrict these shortcuts to only trigger when Shift is not pressed.
| keyEvent.isCtrlPressed && keyEvent.key == Key.B && | |
| keyEvent.isCtrlPressed && !keyEvent.isShiftPressed && keyEvent.key == Key.B && |
Closes #22
What
Adds inline formatting keyboard shortcuts to the editor, dispatched in
TextEditorKeyCommandHandlerand reusing the same span-toggle code path as the markdown toolbar:Ctrl+BCtrl+ICtrl+Shift+XCtrl+EEach shortcut toggles its style over the current selection, or sets a pending style at the cursor for subsequent typing — exactly like clicking the toolbar button.
API
A new
TextFormattingShortcutsdata class maps each shortcut to theSpanStyleit toggles. It's exposed as aformattingShortcutsparameter on bothTextEditorandBasicTextEditor:MarkdownConfiguration.DEFAULTstyles so toggled spans round-trip through the markdown extension.TextFormattingShortcuts.Noneto disable, or set individual styles tonullto unbind a single shortcut.MarkdownConfiguration.Notes
Ctrl+Shift+X(strikethrough) is matched before theCtrl+Xcut branch so it isn't swallowed as a cut.enabled, so they're inert in read-only editors.SpanStyletoggles. Headings/lists/blockquote are line-block operations driven by the markdown extension, not pure span styles, so they're intentionally left out of this change.Tests
FormattingShortcutTestcovers: apply/toggle bold, italic, code, strikethrough-doesn't-cut, pending cursor style with no selection, disabled editor is inert, andNone/unbound is ignored.