Apply further PR Feedback

- Improve settings descriptions in package.json - Get rid of obsolete ViewState and view state-related code - Properly return provider result for contributed trackers - Make 'fireSessionEvent' public as we access it from other classes - Avoid auto-focusing the delay input field if we change options
eclipse-cdt-cloud · May 14, 2024 · 5378008 · 5378008
1 parent 9782d0d
commit 5378008
Show file tree

Hide file tree

Showing 7 changed files with 14 additions and 64 deletions.
diff --git a/package.json b/package.json
@@ -267,7 +267,7 @@
             "Do not automatically refresh when the debugger stops"
           ],
           "default": "on",
-          "description": "Refresh Memory Inspectors when the debugger stops"
+          "description": "Refresh Memory Inspector windows when the debugger stops"
         },
         "memory-inspector.periodicRefresh": {
           "type": "string",
@@ -282,13 +282,13 @@
             "Do not automatically refresh after the configured delay"
           ],
           "default": "off",
-          "description": "Refresh Memory Inspectors after the configured delay"
+          "markdownDescription": "Refresh Memory Inspectors after the configured `#memory-inspector.periodicRefreshInterval#`."
         },
         "memory-inspector.periodicRefreshInterval": {
           "type": "number",
           "default": 500,
           "minimum": 500,
-          "markdownDescription": "Controls the delay in milliseconds after which a Memory Inspector is refrehsed automatically. Only applies when `#memory-inspector.periodicRefresh#` is set to `on`."
+          "markdownDescription": "Controls the delay in milliseconds after which a Memory Inspector is refrehsed automatically. Only applies when `#memory-inspector.periodicRefresh#` is enabled."
         },
         "memory-inspector.groupings.bytesPerMAU": {
           "type": "number",

diff --git a/src/common/messaging.ts b/src/common/messaging.ts
@@ -45,19 +45,13 @@ export interface SessionContext {
     stopped?: boolean;
 }
 
-export interface ViewState {
-    active: boolean;
-    visible: boolean;
-}
-
 // Notifications
 export const readyType: NotificationType<void> = { method: 'ready' };
 export const setMemoryViewSettingsType: NotificationType<Partial<MemoryViewSettings>> = { method: 'setMemoryViewSettings' };
 export const resetMemoryViewSettingsType: NotificationType<void> = { method: 'resetMemoryViewSettings' };
 export const setTitleType: NotificationType<string> = { method: 'setTitle' };
 export const memoryWrittenType: NotificationType<WrittenMemory> = { method: 'memoryWritten' };
 export const sessionContextChangedType: NotificationType<SessionContext> = { method: 'sessionContextChanged' };
-export const viewStateChangedType: NotificationType<ViewState> = { method: 'viewStateChanged' };
 
 // Requests
 export const setOptionsType: RequestType<MemoryOptions, void> = { method: 'setOptions' };

diff --git a/src/plugin/memory-provider.ts b/src/plugin/memory-provider.ts
@@ -39,18 +39,10 @@ export class MemoryProvider {
     }
 
     public activate(context: vscode.ExtensionContext): void {
-        const createDebugAdapterTracker = (session: vscode.DebugSession): Required<vscode.DebugAdapterTracker> => {
+        const createDebugAdapterTracker = (session: vscode.DebugSession): vscode.ProviderResult<vscode.DebugAdapterTracker> => {
             const handlerForSession = this.adapterRegistry.getHandlerForSession(session.type);
             const contributedTracker = handlerForSession?.initializeAdapterTracker?.(session);
-
-            return ({
-                onWillStartSession: () => contributedTracker?.onWillStartSession?.(),
-                onWillStopSession: () => contributedTracker?.onWillStopSession?.(),
-                onDidSendMessage: message => contributedTracker?.onDidSendMessage?.(message),
-                onError: error => contributedTracker?.onError?.(error),
-                onExit: (code, signal) => contributedTracker?.onExit?.(code, signal),
-                onWillReceiveMessage: message => contributedTracker?.onWillReceiveMessage?.(message)
-            });
+            return contributedTracker;
         };
         context.subscriptions.push(vscode.debug.registerDebugAdapterTrackerFactory('*', { createDebugAdapterTracker }));
     }
@@ -68,7 +60,7 @@ export class MemoryProvider {
             const offset = response?.offset ? (args.offset ?? 0) + response.offset : args.offset;
             const count = response?.bytesWritten ?? stringToBytesMemory(args.data).length;
             // if our custom handler is active, let's fire the event ourselves
-            this.sessionTracker['fireSessionEvent'](session, 'memory-written', { memoryReference: args.memoryReference, offset, count });
+            this.sessionTracker.fireSessionEvent(session, 'memory-written', { memoryReference: args.memoryReference, offset, count });
         };
 
         return sendRequest(session, 'writeMemory', args).then(response => {

diff --git a/src/plugin/memory-webview-main.ts b/src/plugin/memory-webview-main.ts
@@ -39,8 +39,6 @@ import {
     showAdvancedOptionsType,
     StoreMemoryArguments,
     storeMemoryType,
-    ViewState,
-    viewStateChangedType,
     WebviewSelection,
     WriteMemoryArguments,
     WriteMemoryResult,
@@ -191,12 +189,11 @@ export class MemoryWebview implements vscode.CustomReadonlyEditorProvider {
         `;
     }
 
-    protected setWebviewMessageListener(panel: vscode.WebviewPanel, initialOptions?: MemoryOptions): void {
+    protected setWebviewMessageListener(panel: vscode.WebviewPanel, options?: MemoryOptions): void {
         const participant = this.messenger.registerWebviewPanel(panel);
-        let memoryOptions = initialOptions;
         const disposables = [
-            this.messenger.onNotification(readyType, () => this.initialize(participant, panel, memoryOptions), { sender: participant }),
-            this.messenger.onRequest(setOptionsType, newOptions => { memoryOptions = { ...memoryOptions, ...newOptions }; }, { sender: participant }),
+            this.messenger.onNotification(readyType, () => this.initialize(participant, panel, options), { sender: participant }),
+            this.messenger.onRequest(setOptionsType, newOptions => { options = { ...options, ...newOptions }; }, { sender: participant }),
             this.messenger.onRequest(logMessageType, message => outputChannelLogger.info('[webview]:', message), { sender: participant }),
             this.messenger.onRequest(readMemoryType, request => this.readMemory(request), { sender: participant }),
             this.messenger.onRequest(writeMemoryType, request => this.writeMemory(request), { sender: participant }),
@@ -207,13 +204,11 @@ export class MemoryWebview implements vscode.CustomReadonlyEditorProvider {
             this.messenger.onRequest(applyMemoryType, () => this.applyMemory(), { sender: participant }),
             this.sessionTracker.onSessionEvent(event => this.handleSessionEvent(participant, event))
         ];
-        panel.onDidChangeViewState(newState => this.setViewState(participant, { active: newState.webviewPanel.active, visible: newState.webviewPanel.visible }));
         panel.onDidDispose(() => disposables.forEach(disposable => disposable.dispose()));
     }
 
     protected async initialize(participant: WebviewIdMessageParticipant, panel: vscode.WebviewPanel, options?: MemoryOptions): Promise<void> {
         this.setSessionContext(participant, this.createContext());
-        this.setViewState(participant, { active: panel.active, visible: panel.visible });
         this.setInitialSettings(participant, panel.title);
         this.refresh(participant, options);
     }
@@ -234,10 +229,6 @@ export class MemoryWebview implements vscode.CustomReadonlyEditorProvider {
         this.messenger.sendNotification(sessionContextChangedType, webviewParticipant, context);
     }
 
-    protected setViewState(webviewParticipant: WebviewIdMessageParticipant, state: ViewState): void {
-        this.messenger.sendNotification(viewStateChangedType, webviewParticipant, state);
-    }
-
     protected getMemoryViewSettings(messageParticipant: WebviewIdMessageParticipant, title: string): MemoryViewSettings {
         const memoryInspectorConfiguration = vscode.workspace.getConfiguration(manifest.PACKAGE_NAME);
         const bytesPerMau = memoryInspectorConfiguration.get<number>(manifest.CONFIG_BYTES_PER_MAU, manifest.DEFAULT_BYTES_PER_MAU);

diff --git a/src/plugin/session-tracker.ts b/src/plugin/session-tracker.ts
@@ -107,7 +107,7 @@ export class SessionTracker implements vscode.DebugAdapterTrackerFactory {
         this._onSessionEvent.fire({ event: 'active', session: session ? this.sessionInfo(session) : undefined });
     }
 
-    protected fireSessionEvent<K extends keyof Omit<SessionEvents, 'active'>>(session: vscode.DebugSession, event: K, data: SessionEvents[K]['data']): void {
+    fireSessionEvent<K extends keyof Omit<SessionEvents, 'active'>>(session: vscode.DebugSession, event: K, data: SessionEvents[K]['data']): void {
         this._onSessionEvent.fire({ event, session: this.sessionInfo(session), data });
     }
 

diff --git a/src/webview/components/options-widget.tsx b/src/webview/components/options-widget.tsx
@@ -53,7 +53,6 @@ export interface OptionsWidgetProps
 
 interface OptionsWidgetState {
     isTitleEditing: boolean;
-    isEnablingPeriodicRefresh: boolean;
 }
 
 const enum InputId {
@@ -104,7 +103,7 @@ export class OptionsWidget extends React.Component<OptionsWidgetProps, OptionsWi
             validate: this.validate,
             onSubmit: () => this.props.fetchMemory(this.props.configuredReadArguments),
         };
-        this.state = { isTitleEditing: false, isEnablingPeriodicRefresh: false };
+        this.state = { isTitleEditing: false };
     }
 
     protected validate = (values: OptionsForm) => {
@@ -132,11 +131,6 @@ export class OptionsWidget extends React.Component<OptionsWidgetProps, OptionsWi
         if (prevProps.activeReadArguments === DEFAULT_READ_ARGUMENTS) {
             this.formConfig.initialErrors = this.validate(this.optionsFormValues);
         }
-        if (!prevState.isEnablingPeriodicRefresh && this.state.isEnablingPeriodicRefresh) {
-            const input = this.refreshRateInput.current?.getElement().getElementsByTagName('input')[0];
-            input?.focus();
-            input?.select();
-        }
     }
 
     override render(): React.ReactNode {
@@ -420,7 +414,7 @@ export class OptionsWidget extends React.Component<OptionsWidgetProps, OptionsWi
                             </div>
 
                             <h2>Refresh</h2>
-                            <div className='flex align-items-center'>
+                            <div className='flex align-items-center mt-2'>
                                 <Checkbox
                                     id={InputId.RefreshOnStop}
                                     onChange={this.handleAdvancedOptionsDropdownChange}
@@ -429,15 +423,15 @@ export class OptionsWidget extends React.Component<OptionsWidgetProps, OptionsWi
                                 <label htmlFor={InputId.ShowRadixPrefix} className='ml-2'>Refresh On Stop</label>
                             </div>
 
-                            <label htmlFor={InputId.PeriodicRefresh} className='advanced-options-label'>Periodic Refresh</label>
+                            <label htmlFor={InputId.PeriodicRefresh} className='advanced-options-label mt-2'>Periodic Refresh</label>
                             <Dropdown
                                 id={InputId.PeriodicRefresh}
                                 value={this.props.periodicRefresh}
                                 onChange={this.handleAdvancedOptionsDropdownChange}
                                 options={[...PERIODIC_REFRESH_CHOICES]}
                                 className="advanced-options-dropdown" />
 
-                            <div className='flex align-items-center'>
+                            <div className='flex align-items-center mt-2'>
                                 <InputNumber
                                     id={InputId.PeriodicRefreshInterval}
                                     ref={this.refreshRateInput}
@@ -548,7 +542,6 @@ export class OptionsWidget extends React.Component<OptionsWidgetProps, OptionsWi
                 break;
             case InputId.PeriodicRefresh:
                 this.props.updateRenderOptions({ periodicRefresh: value });
-                this.setState({ isEnablingPeriodicRefresh: value !== 'off' });
                 break;
             default: {
                 throw new Error(`${id} can not be handled. Did you call the correct method?`);

diff --git a/src/webview/memory-webview-view.tsx b/src/webview/memory-webview-view.tsx
@@ -40,8 +40,6 @@ import {
     setTitleType,
     showAdvancedOptionsType,
     storeMemoryType,
-    ViewState,
-    viewStateChangedType,
     WebviewSelection,
 } from '../common/messaging';
 import { Change, hasChanged, hasChangedTo } from '../common/typescript';
@@ -63,7 +61,6 @@ import { messenger } from './view-messenger';
 export interface MemoryAppState extends MemoryState, MemoryDisplayConfiguration {
     messageParticipant: WebviewIdMessageParticipant;
     title: string;
-    viewState: ViewState;
     sessionContext: SessionContext;
     effectiveAddressLength: number;
     decorations: Decoration[];
@@ -77,11 +74,6 @@ export const DEFAULT_SESSION_CONTEXT: SessionContext = {
     canWrite: false
 };
 
-export const DEFAULT_VIEW_STATE: ViewState = {
-    active: false,
-    visible: false
-};
-
 export const DEFAULT_MEMORY_DISPLAY_CONFIGURATION: MemoryDisplayConfiguration = {
     bytesPerMau: manifest.DEFAULT_BYTES_PER_MAU,
     mausPerGroup: manifest.DEFAULT_MAUS_PER_GROUP,
@@ -113,7 +105,6 @@ class App extends React.Component<{}, MemoryAppState> {
         this.state = {
             messageParticipant: { type: 'webview', webviewId: '' },
             title: 'Memory',
-            viewState: DEFAULT_VIEW_STATE,
             sessionContext: DEFAULT_SESSION_CONTEXT,
             memory: undefined,
             effectiveAddressLength: 0,
@@ -132,7 +123,6 @@ class App extends React.Component<{}, MemoryAppState> {
         messenger.onRequest(setOptionsType, options => this.setOptions(options));
         messenger.onNotification(memoryWrittenType, writtenMemory => this.memoryWritten(writtenMemory));
         messenger.onNotification(sessionContextChangedType, sessionContext => this.sessionContextChanged(sessionContext));
-        messenger.onNotification(viewStateChangedType, viewState => this.viewStateChanged(viewState));
         messenger.onNotification(setMemoryViewSettingsType, config => {
             if (config.visibleColumns) {
                 for (const column of columnContributionService.getColumns()) {
@@ -167,11 +157,6 @@ class App extends React.Component<{}, MemoryAppState> {
         if (current.refreshOnStop === 'on' && hasChangedTo(sessionContextChange, 'stopped', true)) {
             this.fetchMemory();
         }
-        // activate the code below if you want to refresh the memory when the view becomes active (focussed)
-        // const viewStateChange: Change<ViewState> = { from: from.viewState, to: current.viewState };
-        // if (hasChangedTo(viewStateChange, 'active', true)) {
-        //     this.fetchMemory();
-        // }
 
         hoverService.setMemoryState(this.state);
     }
@@ -231,10 +216,6 @@ class App extends React.Component<{}, MemoryAppState> {
         this.setState({ sessionContext });
     }
 
-    protected viewStateChanged(viewState: ViewState): void {
-        this.setState({ viewState });
-    }
-
     public render(): React.ReactNode {
         return <PrimeReactProvider>
             <MemoryWidget
@@ -329,7 +310,6 @@ class App extends React.Component<{}, MemoryAppState> {
         } finally {
             this.setState({ isMemoryFetching: false });
         }
-
     }
 
     protected getEffectiveAddressLength(memory?: Memory): number {