debugger: Add keyboard shortcuts for pause/resume, stepping, and frame navigation (#3867)#9835
Open
NadeemIqbal wants to merge 2 commits into
Open
Conversation
…e navigation Wire up the F8 / F9 / F10 / F11 / Shift+F11 shortcuts requested in flutter#3867 to the existing DebuggerController actions, mirroring the bindings used by Chrome DevTools and VS Code: - F8 — Pause when running, Resume when paused. - F9 — Cycle the selected frame in the Call Stack (wraps to top). - F10 — Step Over. - F11 — Step In. - Shift+F11 — Step Out. Each shortcut is gated by the same conditions that disable the corresponding debugger control buttons in `controls.dart`: - Pause/Resume is disabled on a system isolate, and disabled while a resume is already in flight when the isolate is paused. - Step Over/In/Out require a paused, non-system isolate with at least one stack frame and no in-flight resume. - Next frame requires more than one frame to be available. Adds a unit test that exercises each Action's `isEnabled` and `invoke` directly so the gating logic is covered without depending on a pumped widget tree. Fixes flutter#3867.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements keyboard shortcuts for debugger stepping and pause/resume operations (F8 through F11), matching the behavior of Chrome DevTools and VS Code. The changes include new intents and actions in debugger_screen.dart, key set definitions in key_sets.dart, and comprehensive regression tests. Feedback identifies an opportunity to refactor duplicated stepping logic into DebuggerController and suggests removing a redundant frame count check in NextStackFrameAction.invoke.
Comment on lines
+435
to
+440
| bool _canStep(DebuggerController controller) { | ||
| return serviceConnection.serviceManager.isMainIsolatePaused && | ||
| !controller.resuming.value && | ||
| controller.stackFramesWithLocation.value.isNotEmpty && | ||
| !controller.isSystemIsolate; | ||
| } |
Contributor
There was a problem hiding this comment.
[CONCERN] The logic for determining if stepping is allowed is duplicated here and in controls.dart. To improve maintainability and ensure consistent behavior between buttons and shortcuts, this logic should be moved to a shared property or method in DebuggerController.
References
- Code Reuse: Use shared primitives and components rather than recreating them from scratch. (link)
Comment on lines
+489
to
+492
| if (frames.length < 2) { | ||
| return; | ||
| } | ||
| final currentlySelected = controller.selectedStackFrame.value; |
Contributor
There was a problem hiding this comment.
[NIT] This check is redundant as isEnabled already ensures that frames.length > 1. Removing it simplifies the method.
final currentlySelected = controller.selectedStackFrame.value;References
- Adhere to DRY (Don't Repeat Yourself) principles by removing redundant logic checks. (link)
… guard Address review feedback on flutter#9835: - The condition for "step operations are allowed" was duplicated between the debugger control buttons in `controls.dart` and the keyboard-shortcut Actions in `debugger_screen.dart`. Expose it once as `DebuggerController.canStep` and have both surfaces read it, so they cannot drift out of sync. - `PauseResumeAction.isEnabled` now reads `intent._controller.isSystemIsolate` directly; the `_isSystemIsolate` helper was only there to mirror the duplicated style and is no longer needed. - `NextStackFrameAction.invoke` had a `frames.length < 2` early return that was redundant with the matching guard in `isEnabled`; drop it. The Step Action tests now stub `canStep` directly instead of driving it through its inputs — that is what the Actions actually read, and the component conditions are tested where they belong (on `DebuggerController`).
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.
Fixes #3867.
Description
Wires up the keyboard shortcuts requested in #3867 for the debugger panel, using the bindings Elliette suggested on the issue (which match Chrome DevTools and the stepping triplet used by VS Code):
F8F9F10F11Shift+F11The new shortcuts plug into the existing
ShortcutsConfigurationreturned byDebuggerScreen.buildKeyboardShortcuts, alongside the existing Go-To-Line / Search-In-File / Open-File / Escape shortcuts. Each action delegates to the sameDebuggerControllermethods used by the on-screen buttons (pause(),resume(),stepOver(),stepIn(),stepOut(),selectStackFrame()).Each action's
isEnabledmirrors the gating already used by the buttons incontrols.dart:When a shortcut's
isEnabledreturnsfalseit falls through to the framework's default key handling, so the shortcut does not steal events when the debugger is not in a stepable state.Note on previous-frame navigation
The issue lists "Call stack navigation (next / previous frame)" but the comment specifies only
F9. This PR implements next-frame onF9(cycling through the call stack, which gives previous-frame behaviour after a wrap). Happy to add an explicitShift+F9for previous-frame in a follow-up if that's preferred.Tests
Added
packages/devtools_app/test/screens/debugger/debugger_keyboard_shortcuts_test.dart, which exercises each Action'sisEnabledandinvokedirectly against a mockedDebuggerController. This covers:isMainIsolatePaused.Pre-launch Checklist