fix: various debug fixes and VS Code compatibility enhancements (#11984)

* fix: various debug fixes and VS Code compatibility enhancements - feat: added support for `debug/toolbar` and `debug/variables/context`, - feat: added support for `debugState` when context (#11871), - feat: can customize debug session timeout, and error handling (#11879), - fix: the `debugType` context that is not updated, - fix: `configure` must happen after receiving capabilities (#11886), - fix: added missing conext menu in the _Variables_ view, - fix: handle `setFunctionBreakboints` response with no `body` (#11885), - fix: `DebugExt` fires `didStart` event on `didCreate` (#11916), - fix: validate editor selection based on the text model (#11880) Signed-off-by: Akos Kitta <[email protected]> * fix: use when context for command node filtering Signed-off-by: Akos Kitta <[email protected]> * chore: removed test debug VSIX Signed-off-by: Akos Kitta <[email protected]> * fix: revert `timeout` check Signed-off-by: Akos Kitta <[email protected]> * fix: clarification on the possible `'debugState'` Signed-off-by: Akos Kitta <[email protected]> * fix: use hard-coded debugger `clientID` and `clientName` Signed-off-by: Akos Kitta <[email protected]> * fix: use review-requested method name Signed-off-by: Akos Kitta <[email protected]> * fix: changed method name + added doc Signed-off-by: Akos Kitta <[email protected]> * fix: `stopTimeout` is a default `ctor` argument Signed-off-by: Akos Kitta <[email protected]> * fix: incorrect method name Signed-off-by: Akos Kitta <[email protected]> * fix: both `didCreate` and `didStart` must be API Signed-off-by: Akos Kitta <[email protected]> * fix: call both on create and start Signed-off-by: Akos Kitta <[email protected]> * fix: workaround for microsoft/vscode-mock-debug#85 Signed-off-by: Akos Kitta <[email protected]> * simplify writing The collection of contributed commands was written in a convoluted way, this commit makes it more straightforward with just 2 loops. * use `Math.max` to clamp values into positives The ternary implementation is stricly equivalent to the `Math.max` function, so use that instead. * fix default option value handling Assigning a default option object to set default values is problematic as said default values will be discarded if any option object is passed. Instead default values must be handled in conjuction of whatever options are passed to functions. Signed-off-by: Akos Kitta <[email protected]> Co-authored-by: Paul Maréchal <[email protected]>
eclipse-theia · Jan 24, 2023 · 678e335 · 678e335
1 parent f925bdb
commit 678e335
Show file tree

Hide file tree

Showing 14 changed files with 207 additions and 38 deletions.
diff --git a/packages/debug/src/browser/debug-session-manager.ts b/packages/debug/src/browser/debug-session-manager.ts
@@ -27,7 +27,7 @@ import { DebugConfiguration } from '../common/debug-common';
 import { DebugError, DebugService } from '../common/debug-service';
 import { BreakpointManager } from './breakpoint/breakpoint-manager';
 import { DebugConfigurationManager } from './debug-configuration-manager';
-import { DebugSession, DebugState } from './debug-session';
+import { DebugSession, DebugState, debugStateContextValue } from './debug-session';
 import { DebugSessionContributionRegistry, DebugSessionFactory } from './debug-session-contribution';
 import { DebugCompoundRoot, DebugCompoundSessionOptions, DebugConfigurationSessionOptions, DebugSessionOptions, InternalDebugSessionOptions } from './debug-session-options';
 import { DebugStackFrame } from './model/debug-stack-frame';
@@ -106,7 +106,9 @@ export class DebugSessionManager {
     protected readonly onDidChangeEmitter = new Emitter<DebugSession | undefined>();
     readonly onDidChange: Event<DebugSession | undefined> = this.onDidChangeEmitter.event;
     protected fireDidChange(current: DebugSession | undefined): void {
+        this.debugTypeKey.set(current?.configuration.type);
         this.inDebugModeKey.set(this.inDebugMode);
+        this.debugStateKey.set(debugStateContextValue(this.state));
         this.onDidChangeEmitter.fire(current);
     }
 
@@ -154,11 +156,13 @@ export class DebugSessionManager {
 
     protected debugTypeKey: ContextKey<string>;
     protected inDebugModeKey: ContextKey<boolean>;
+    protected debugStateKey: ContextKey<string>;
 
     @postConstruct()
     protected init(): void {
         this.debugTypeKey = this.contextKeyService.createKey<string>('debugType', undefined);
         this.inDebugModeKey = this.contextKeyService.createKey<boolean>('inDebugMode', this.inDebugMode);
+        this.debugStateKey = this.contextKeyService.createKey<string>('debugState', debugStateContextValue(this.state));
         this.breakpoints.onDidChangeMarkers(uri => this.fireDidChangeBreakpoints({ uri }));
         this.labelProvider.onDidChange(event => {
             for (const uriString of this.breakpoints.getUris()) {

diff --git a/packages/debug/src/browser/debug-session.tsx b/packages/debug/src/browser/debug-session.tsx
@@ -50,6 +50,18 @@ export enum DebugState {
     Running,
     Stopped
 }
+/**
+ * The mapped string values must not change as they are used for the `debugState` when context closure.
+ * For more details see the `Debugger contexts` section of the [official doc](https://code.visualstudio.com/api/references/when-clause-contexts#available-contexts).
+ */
+export function debugStateContextValue(state: DebugState): string {
+    switch (state) {
+        case DebugState.Initializing: return 'initializing';
+        case DebugState.Stopped: return 'stopped';
+        case DebugState.Running: return 'running';
+        default: return 'inactive';
+    }
+}
 
 // FIXME: make injectable to allow easily inject services
 export class DebugSession implements CompositeTreeElement {
@@ -74,7 +86,7 @@ export class DebugSession implements CompositeTreeElement {
     protected readonly childSessions = new Map<string, DebugSession>();
     protected readonly toDispose = new DisposableCollection();
 
-    private isStopping: boolean = false;
+    protected isStopping: boolean = false;
 
     constructor(
         readonly id: string,
@@ -89,6 +101,10 @@ export class DebugSession implements CompositeTreeElement {
         protected readonly fileService: FileService,
         protected readonly debugContributionProvider: ContributionProvider<DebugContribution>,
         protected readonly workspaceService: WorkspaceService,
+        /**
+         * Number of millis after a `stop` request times out. It's 5 seconds by default.
+         */
+        protected readonly stopTimeout = 5_000,
     ) {
         this.connection.onRequest('runInTerminal', (request: DebugProtocol.RunInTerminalRequest) => this.runInTerminal(request));
         this.connection.onDidClose(() => {
@@ -274,19 +290,25 @@ export class DebugSession implements CompositeTreeElement {
     }
 
     protected async initialize(): Promise<void> {
-        const response = await this.connection.sendRequest('initialize', {
-            clientID: 'Theia',
-            clientName: 'Theia IDE',
-            adapterID: this.configuration.type,
-            locale: 'en-US',
-            linesStartAt1: true,
-            columnsStartAt1: true,
-            pathFormat: 'path',
-            supportsVariableType: false,
-            supportsVariablePaging: false,
-            supportsRunInTerminalRequest: true
-        });
-        this.updateCapabilities(response?.body || {});
+        try {
+            const response = await this.connection.sendRequest('initialize', {
+                clientID: 'Theia',
+                clientName: 'Theia IDE',
+                adapterID: this.configuration.type,
+                locale: 'en-US',
+                linesStartAt1: true,
+                columnsStartAt1: true,
+                pathFormat: 'path',
+                supportsVariableType: false,
+                supportsVariablePaging: false,
+                supportsRunInTerminalRequest: true
+            });
+            this.updateCapabilities(response?.body || {});
+            this.didReceiveCapabilities.resolve();
+        } catch (err) {
+            this.didReceiveCapabilities.reject(err);
+            throw err;
+        }
     }
 
     protected async launchOrAttach(): Promise<void> {
@@ -304,8 +326,17 @@ export class DebugSession implements CompositeTreeElement {
         }
     }
 
+    /**
+     * The `send('initialize')` request could resolve later than `on('initialized')` emits the event.
+     * Hence, the `configure` would use the empty object `capabilities`.
+     * Using the empty `capabilities` could result in missing exception breakpoint filters, as
+     * always `capabilities.exceptionBreakpointFilters` is falsy. This deferred promise works
+     * around this timing issue. https://github.com/eclipse-theia/theia/issues/11886
+     */
+    protected didReceiveCapabilities = new Deferred<void>();
     protected initialized = false;
     protected async configure(): Promise<void> {
+        await this.didReceiveCapabilities.promise;
         if (this.capabilities.exceptionBreakpointFilters) {
             const exceptionBreakpoints = [];
             for (const filter of this.capabilities.exceptionBreakpointFilters) {
@@ -340,24 +371,39 @@ export class DebugSession implements CompositeTreeElement {
         if (!this.isStopping) {
             this.isStopping = true;
             if (this.canTerminate()) {
-                const terminated = this.waitFor('terminated', 5000);
+                const terminated = this.waitFor('terminated', this.stopTimeout);
                 try {
-                    await this.connection.sendRequest('terminate', { restart: isRestart }, 5000);
+                    await this.connection.sendRequest('terminate', { restart: isRestart }, this.stopTimeout);
                     await terminated;
                 } catch (e) {
-                    console.error('Did not receive terminated event in time', e);
+                    this.handleTerminateError(e);
                 }
             } else {
+                const terminateDebuggee = this.initialized && this.capabilities.supportTerminateDebuggee;
                 try {
-                    await this.sendRequest('disconnect', { restart: isRestart }, 5000);
+                    await this.sendRequest('disconnect', { restart: isRestart, terminateDebuggee }, this.stopTimeout);
                 } catch (e) {
-                    console.error('Error on disconnect', e);
+                    this.handleDisconnectError(e);
                 }
             }
             callback();
         }
     }
 
+    /**
+     * Invoked when sending the `terminate` request to the debugger is rejected or timed out.
+     */
+    protected handleTerminateError(err: unknown): void {
+        console.error('Did not receive terminated event in time', err);
+    }
+
+    /**
+     * Invoked when sending the `disconnect` request to the debugger is rejected or timed out.
+     */
+    protected handleDisconnectError(err: unknown): void {
+        console.error('Error on disconnect', err);
+    }
+
     async disconnect(isRestart: boolean, callback: () => void): Promise<void> {
         if (!this.isStopping) {
             this.isStopping = true;
@@ -665,12 +711,17 @@ export class DebugSession implements CompositeTreeElement {
                 const response = await this.sendRequest('setFunctionBreakpoints', {
                     breakpoints: enabled.map(b => b.origin.raw)
                 });
-                response.body.breakpoints.forEach((raw, index) => {
-                    // node debug adapter returns more breakpoints sometimes
-                    if (enabled[index]) {
-                        enabled[index].update({ raw });
-                    }
-                });
+                // Apparently, `body` and `breakpoints` can be missing.
+                // https://github.com/eclipse-theia/theia/issues/11885
+                // https://github.com/microsoft/vscode/blob/80004351ccf0884b58359f7c8c801c91bb827d83/src/vs/workbench/contrib/debug/browser/debugSession.ts#L448-L449
+                if (response && response.body) {
+                    response.body.breakpoints.forEach((raw, index) => {
+                        // node debug adapter returns more breakpoints sometimes
+                        if (enabled[index]) {
+                            enabled[index].update({ raw });
+                        }
+                    });
+                }
             } catch (error) {
                 // could be error or promise rejection of DebugProtocol.SetFunctionBreakpoints
                 if (error instanceof Error) {
@@ -699,10 +750,12 @@ export class DebugSession implements CompositeTreeElement {
         );
         const enabled = all.filter(b => b.enabled);
         try {
+            const breakpoints = enabled.map(({ origin }) => origin.raw);
             const response = await this.sendRequest('setBreakpoints', {
                 source: source.raw,
                 sourceModified,
-                breakpoints: enabled.map(({ origin }) => origin.raw)
+                breakpoints,
+                lines: breakpoints.map(({ line }) => line)
             });
             response.body.breakpoints.forEach((raw, index) => {
                 // node debug adapter returns more breakpoints sometimes

diff --git a/packages/debug/src/browser/model/debug-stack-frame.tsx b/packages/debug/src/browser/model/debug-stack-frame.tsx
@@ -64,28 +64,36 @@ export class DebugStackFrame extends DebugStackFrameData implements TreeElement
         }));
     }
 
-    async open(options: WidgetOpenerOptions = {
-        mode: 'reveal'
-    }): Promise<EditorWidget | undefined> {
+    async open(options?: WidgetOpenerOptions): Promise<EditorWidget | undefined> {
         if (!this.source) {
             return undefined;
         }
         const { line, column, endLine, endColumn } = this.raw;
         const selection: RecursivePartial<Range> = {
-            start: Position.create(line - 1, column - 1)
+            start: Position.create(this.clampPositive(line - 1), this.clampPositive(column - 1))
         };
         if (typeof endLine === 'number') {
             selection.end = {
-                line: endLine - 1,
-                character: typeof endColumn === 'number' ? endColumn - 1 : undefined
+                line: this.clampPositive(endLine - 1),
+                character: typeof endColumn === 'number' ? this.clampPositive(endColumn - 1) : undefined
             };
         }
         this.source.open({
+            mode: 'reveal',
             ...options,
             selection
         });
     }
 
+    /**
+     * Debugger can send `column: 0` value despite of initializing the debug session with `columnsStartAt1: true`.
+     * This method can be used to ensure that neither `column` nor `column` are negative numbers.
+     * See https://github.com/microsoft/vscode-mock-debug/issues/85.
+     */
+    protected clampPositive(value: number): number {
+        return Math.max(value, 0);
+    }
+
     protected scopes: Promise<DebugScope[]> | undefined;
     getScopes(): Promise<DebugScope[]> {
         return this.scopes || (this.scopes = this.doGetScopes());

diff --git a/packages/debug/src/browser/style/index.css b/packages/debug/src/browser/style/index.css
@@ -146,6 +146,17 @@
     opacity: 1;
 }
 
+.debug-toolbar .debug-action>div {
+    font-family: var(--theia-ui-font-family);
+    font-size: var(--theia-ui-font-size0);
+    display: flex;
+    align-items: center;
+    align-self: center;
+    justify-content: center;
+    text-align: center;
+    min-height: inherit;
+}
+
 /** Console */
 
 #debug-console .theia-console-info {

diff --git a/packages/debug/src/browser/view/debug-action.tsx b/packages/debug/src/browser/view/debug-action.tsx
@@ -21,15 +21,20 @@ export class DebugAction extends React.Component<DebugAction.Props> {
 
     override render(): React.ReactNode {
         const { enabled, label, iconClass } = this.props;
-        const classNames = ['debug-action', ...codiconArray(iconClass, true)];
+        const classNames = ['debug-action'];
+        if (iconClass) {
+            classNames.push(...codiconArray(iconClass, true));
+        }
         if (enabled === false) {
             classNames.push(DISABLED_CLASS);
         }
         return <span tabIndex={0}
             className={classNames.join(' ')}
             title={label}
             onClick={this.props.run}
-            ref={this.setRef} />;
+            ref={this.setRef} >
+            {!iconClass && <div>{label}</div>}
+        </span>;
     }
 
     focus(): void {

diff --git a/packages/debug/src/browser/view/debug-toolbar-widget.tsx b/packages/debug/src/browser/view/debug-toolbar-widget.tsx
@@ -16,7 +16,8 @@
 
 import * as React from '@theia/core/shared/react';
 import { inject, postConstruct, injectable } from '@theia/core/shared/inversify';
-import { Disposable, DisposableCollection, MenuPath } from '@theia/core';
+import { CommandMenuNode, CommandRegistry, CompoundMenuNode, Disposable, DisposableCollection, MenuModelRegistry, MenuPath } from '@theia/core';
+import { ContextKeyService } from '@theia/core/lib/browser/context-key-service';
 import { ReactWidget } from '@theia/core/lib/browser/widgets';
 import { DebugViewModel } from './debug-view-model';
 import { DebugState } from '../debug-session';
@@ -28,8 +29,10 @@ export class DebugToolBar extends ReactWidget {
 
     static readonly MENU: MenuPath = ['debug-toolbar-menu'];
 
-    @inject(DebugViewModel)
-    protected readonly model: DebugViewModel;
+    @inject(CommandRegistry) protected readonly commandRegistry: CommandRegistry;
+    @inject(MenuModelRegistry) protected readonly menuModelRegistry: MenuModelRegistry;
+    @inject(ContextKeyService) protected readonly contextKeyService: ContextKeyService;
+    @inject(DebugViewModel) protected readonly model: DebugViewModel;
 
     protected readonly onRender = new DisposableCollection();
 
@@ -65,6 +68,7 @@ export class DebugToolBar extends ReactWidget {
     protected render(): React.ReactNode {
         const { state } = this.model;
         return <React.Fragment>
+            {this.renderContributedCommands()}
             {this.renderContinue()}
             <DebugAction enabled={state === DebugState.Stopped} run={this.stepOver} label={nls.localizeByDefault('Step Over')}
                 iconClass='debug-step-over' ref={this.setStepRef} />
@@ -77,6 +81,43 @@ export class DebugToolBar extends ReactWidget {
             {this.renderStart()}
         </React.Fragment>;
     }
+
+    protected renderContributedCommands(): React.ReactNode {
+        const debugActions: React.ReactNode[] = [];
+        // first, search for CompoundMenuNodes:
+        this.menuModelRegistry.getMenu(DebugToolBar.MENU).children.forEach(compoundMenuNode => {
+            if (CompoundMenuNode.is(compoundMenuNode) && this.matchContext(compoundMenuNode.when)) {
+                // second, search for nested CommandMenuNodes:
+                compoundMenuNode.children.forEach(commandMenuNode => {
+                    if (CommandMenuNode.is(commandMenuNode) && this.matchContext(commandMenuNode.when)) {
+                        debugActions.push(this.debugAction(commandMenuNode));
+                    }
+                });
+            }
+        });
+        return debugActions;
+    }
+
+    protected matchContext(when?: string): boolean {
+        return !when || this.contextKeyService.match(when);
+    }
+
+    protected debugAction(commandMenuNode: CommandMenuNode): React.ReactNode {
+        const { command, icon = '', label = '' } = commandMenuNode;
+        if (!label && !icon) {
+            const { when } = commandMenuNode;
+            console.warn(`Neither 'label' nor 'icon' properties were defined for the command menu node. (${JSON.stringify({ command, when })}}. Skipping.`);
+            return;
+        }
+        const run = () => this.commandRegistry.executeCommand(command);
+        return <DebugAction
+            key={command}
+            enabled={true}
+            label={label}
+            iconClass={icon}
+            run={run} />;
+    }
+
     protected renderStart(): React.ReactNode {
         const { state } = this.model;
         if (state === DebugState.Inactive && this.model.sessionCount === 1) {

diff --git a/packages/editor/src/browser/editor-manager.ts b/packages/editor/src/browser/editor-manager.ts
@@ -253,6 +253,12 @@ export class EditorManager extends NavigatableWidgetOpenHandler<EditorWidget> {
 
     protected getSelection(widget: EditorWidget, selection: RecursivePartial<Range>): Range | Position | undefined {
         const { start, end } = selection;
+        if (Position.is(start)) {
+            if (Position.is(end)) {
+                return widget.editor.document.toValidRange({ start, end });
+            }
+            return widget.editor.document.toValidPosition(start);
+        }
         const line = start && start.line !== undefined && start.line >= 0 ? start.line : undefined;
         if (line === undefined) {
             return undefined;