Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve naming and allow renaming of memory view instances #69

Merged
merged 4 commits into from
Feb 22, 2024
Merged

Improve naming and allow renaming of memory view instances #69

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a176588
Improve naming and allow renaming of memory view instances
planger Feb 14, 2024
b02e96b
Address review comments
planger Feb 21, 2024
5e1ef08
Merge remote-tracking branch 'origin' into issues/61
planger Feb 21, 2024
9fe501f
Address review comments
planger Feb 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions media/options-widget.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,47 @@
* SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
********************************************************************************/

.memory-options-widget {
margin-bottom: 8px;
padding-bottom: 4px;
}

.memory-options-widget .title-container {
display: flex;
align-items: center;
}

.memory-options-widget .title-container h1 {
flex-grow: 1;
font-size: 1.3em;
margin: 8px 0 4px;
}

.memory-options-widget .title-container input {
margin: 5px 0 0;
flex-grow: 1;
}

.memory-options-widget .edit-label-toggle {
position: absolute;
right: 24px;
top: 8px;
opacity: 0;
transition: opacity 0.2s;
}

.memory-options-widget .title-container:hover .edit-label-toggle {
opacity: 1;
}

.core-options {
display: flex;
flex-direction: row;
align-items: end;
margin: 6px 0;
flex-wrap: wrap;
border-bottom: 1px solid var(--vscode-widget-border);
border-top: 1px solid var(--vscode-widget-border);
}

.form-options {
Expand Down
1 change: 1 addition & 0 deletions src/common/messaging.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export const readyType: NotificationType<void> = { method: 'ready' };
export const logMessageType: RequestType<string, void> = { method: 'logMessage' };
export const setMemoryViewSettingsType: NotificationType<Partial<MemoryViewSettings>> = { method: 'setMemoryViewSettings' };
export const resetMemoryViewSettingsType: NotificationType<void> = { method: 'resetMemoryViewSettings' };
export const setTitleType: NotificationType<string> = { method: 'setTitle' };
export const setOptionsType: RequestType<Partial<DebugProtocol.ReadMemoryArguments | undefined>, void> = { method: 'setOptions' };
export const readMemoryType: RequestType<DebugProtocol.ReadMemoryArguments, MemoryReadResult> = { method: 'readMemory' };
export const writeMemoryType: RequestType<DebugProtocol.WriteMemoryArguments, MemoryWriteResult> = { method: 'writeMemory' };
Expand Down
13 changes: 11 additions & 2 deletions src/plugin/memory-webview-main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
getVariables,
setMemoryViewSettingsType,
resetMemoryViewSettingsType,
setTitleType,
} from '../common/messaging';
import { MemoryProvider } from './memory-provider';
import { outputChannelLogger } from './logger';
Expand Down Expand Up @@ -62,6 +63,8 @@ export class MemoryWebview implements vscode.CustomReadonlyEditorProvider {
protected messenger: Messenger;
protected refreshOnStop: RefreshEnum;

protected panelIndices: number = 1;

public constructor(protected extensionUri: vscode.Uri, protected memoryProvider: MemoryProvider) {
this.messenger = new Messenger();

Expand Down Expand Up @@ -125,7 +128,7 @@ export class MemoryWebview implements vscode.CustomReadonlyEditorProvider {
};

if (!panel) {
panel = vscode.window.createWebviewPanel(MemoryWebview.ViewType, 'Memory Inspector', vscode.ViewColumn.Active, options);
panel = vscode.window.createWebviewPanel(MemoryWebview.ViewType, `Memory ${this.panelIndices++}`, vscode.ViewColumn.Active, options);
} else {
panel.webview.options = options;
}
Expand All @@ -137,7 +140,8 @@ export class MemoryWebview implements vscode.CustomReadonlyEditorProvider {
// and executes code based on the message that is received
const webviewParticipant = this.setWebviewMessageListener(panel, initialMemory);

// initialize with configuration
// initialize web view content
this.setTitle(webviewParticipant, panel.title);
this.setInitialSettings(webviewParticipant);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that both of these lines should be moved into the handler of the readyType notification handler in setWebviewMessageListener. Otherwise, there's no guarantee that the code that sets up the listeners to handle these requests will have run inside the webview - I suspect that that's why I'm seeing just Memory in the webview I created rather than the intended title. This also suggests that we should probably keep all of the message handling centralized in the App class, even if it's a bit inconvenient: that at least provides the guarantee that the messaging is synchronized.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent suggestion! Done! 👍

}

Expand Down Expand Up @@ -191,6 +195,7 @@ export class MemoryWebview implements vscode.CustomReadonlyEditorProvider {
this.messenger.onRequest(writeMemoryType, request => this.writeMemory(request), { sender: participant }),
this.messenger.onRequest(getVariables, request => this.getVariables(request), { sender: participant }),
this.messenger.onNotification(resetMemoryViewSettingsType, () => this.setInitialSettings(participant), { sender: participant }),
this.messenger.onNotification(setTitleType, title => { panel.title = title; }, { sender: participant }),

this.memoryProvider.onDidStopDebug(() => {
if (this.refreshOnStop === RefreshEnum.on) {
Expand All @@ -208,6 +213,10 @@ export class MemoryWebview implements vscode.CustomReadonlyEditorProvider {
return participant;
}

protected setTitle(webviewParticipant: WebviewIdMessageParticipant, title: string): void {
this.messenger.sendNotification(setTitleType, webviewParticipant, title);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The machinery to set title from plugin -> webview feels a little heavy: at the moment, it's only used on panel initialization (and may not be working correctly, since in my test, the panel showed the default Memory rather than Memory + Index).

On the other hand I don't actually have a suggestion here - see my next comment.

Copy link
Contributor Author

@planger planger Feb 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, it indeed is a bit heavy-weight. But I'm not aware of a mechanism to store state across webview instances other than in the extension "main". However, as you've suggested, I've now put updating the title into the setMemoryViewSettingsType message.


protected setInitialSettings(webviewParticipant: WebviewIdMessageParticipant): void {
this.messenger.sendNotification(setMemoryViewSettingsType, webviewParticipant, this.getMemoryViewSettings());
}
Expand Down
4 changes: 4 additions & 0 deletions src/webview/components/memory-widget.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import { OptionsWidget } from './options-widget';

interface MemoryWidgetProps extends MemoryDisplayConfiguration {
memory?: Memory;
initialTitle: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name is a little confusing - it's really just title, since we don't distinguish between initialTitle and later titles.

It might be possible to avoid prop drilling here and set up the logic for handling the title in the OptionsWidget, since that's the only place it's relevant, but see my next comment for a caveat.

Copy link
Contributor Author

@planger planger Feb 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've renamed initialTitle into title according to your suggestion, which makes a lot of sense especially now when we avoid the need for storing the title as a state in the OptionsWidget. 👍

Regarding the prop drilling... I didn't find a great approach to avoid that though. I personally have a slight preference for keeping the handling of messenger events in a central place on both ends (i.e. the App for the webview part) to avoid having handlers of messages from the "outside" scattered throughout all react components, but this is just a personal preference.

One way of avoiding prop drilling might be to pull up the header into its own component and import it in the MemoryWidget directly, or even removing the middle layer MemoryWidget altogether. Grouping prop-values into interfaces may also help to achieve better clarity against an exploding number of props. However, I feel this might be something to attack in a separate PR, wdyt?

decorations: Decoration[];
columns: ColumnStatus[];
memoryReference: string;
Expand All @@ -34,6 +35,7 @@ interface MemoryWidgetProps extends MemoryDisplayConfiguration {
toggleColumn(id: string, active: boolean): void;
updateMemoryDisplayConfiguration: (memoryArguments: Partial<MemoryDisplayConfiguration>) => void;
resetMemoryDisplayConfiguration: () => void;
updateTitle: (title: string) => void;
fetchMemory(partialOptions?: Partial<DebugProtocol.ReadMemoryArguments>): Promise<void>
}

Expand Down Expand Up @@ -64,9 +66,11 @@ export class MemoryWidget extends React.Component<MemoryWidgetProps, MemoryWidge
wordSize={this.state.wordSize}
wordsPerGroup={this.props.wordsPerGroup}
groupsPerRow={this.props.groupsPerRow}
initialTitle={this.props.initialTitle}
updateMemoryArguments={this.props.updateMemoryArguments}
updateRenderOptions={this.props.updateMemoryDisplayConfiguration}
resetRenderOptions={this.props.resetMemoryDisplayConfiguration}
updateTitle={this.props.updateTitle}
refreshMemory={this.props.refreshMemory}
toggleColumn={this.props.toggleColumn}
/>
Expand Down
91 changes: 89 additions & 2 deletions src/webview/components/options-widget.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import { Dropdown, DropdownChangeEvent } from 'primereact/dropdown';
import { InputText } from 'primereact/inputtext';
import { OverlayPanel } from 'primereact/overlaypanel';
import { classNames } from 'primereact/utils';
import React, { KeyboardEvent, MouseEventHandler } from 'react';
import React, { ChangeEventHandler, FocusEventHandler, KeyboardEvent, KeyboardEventHandler, MouseEventHandler } from 'react';
import { TableRenderOptions } from '../columns/column-contribution-service';
import {
SerializedTableRenderOptions,
Expand All @@ -31,15 +31,23 @@ import { MultiSelectWithLabel } from './multi-select';
export interface OptionsWidgetProps
extends Omit<TableRenderOptions, 'scrollingBehavior'>,
Required<DebugProtocol.ReadMemoryArguments> {
initialTitle: string;
updateRenderOptions: (options: Partial<SerializedTableRenderOptions>) => void;
resetRenderOptions: () => void;
updateTitle: (title: string) => void;
updateMemoryArguments: (
memoryArguments: Partial<DebugProtocol.ReadMemoryArguments>
) => void;
refreshMemory: () => void;
toggleColumn(id: string, isVisible: boolean): void;
}

interface OptionsWidgetState {
title: string;
previousTitle?: string;
isTitleEditing: boolean;
}

const enum InputId {
Address = 'address',
Offset = 'offset',
Expand All @@ -57,9 +65,10 @@ interface OptionsForm {
const allowedBytesPerGroup = [1, 2, 4, 8, 16];
const allowedGroupsPerRow = [1, 2, 4, 8, 16, 32];

export class OptionsWidget extends React.Component<OptionsWidgetProps, {}> {
export class OptionsWidget extends React.Component<OptionsWidgetProps, OptionsWidgetState> {
protected formConfig: FormikConfig<OptionsForm>;
protected extendedOptions = React.createRef<OverlayPanel>();
protected labelEditInput = React.createRef<HTMLInputElement>();

protected get optionsFormValues(): OptionsForm {
return {
Expand All @@ -80,6 +89,10 @@ export class OptionsWidget extends React.Component<OptionsWidgetProps, {}> {
this.props.refreshMemory();
},
};
this.state = {
title: this.props.initialTitle,
isTitleEditing: false,
};
}

protected validate = (values: OptionsForm) => {
Expand Down Expand Up @@ -117,11 +130,48 @@ export class OptionsWidget extends React.Component<OptionsWidgetProps, {}> {
return errors;
};

componentDidUpdate(prevProps: Readonly<OptionsWidgetProps>, prevState: Readonly<OptionsWidgetState>): void {
if (this.props.initialTitle !== prevProps.initialTitle) {
this.setState({ title: this.props.initialTitle });
}
if (!prevState.isTitleEditing && this.state.isTitleEditing) {
this.labelEditInput.current?.focus();
this.labelEditInput.current?.select();
}
}

override render(): React.ReactNode {
this.formConfig.initialValues = this.optionsFormValues;
const isLabelEditing = this.state.isTitleEditing;

return (
<div className='memory-options-widget px-4'>
<div className='title-container'>
<InputText
ref={this.labelEditInput}
type='text'
value={this.state.title}
onChange={this.handleTitleEdit}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be possible to simplify the state handling here (and avoid storing previousTitle) by not storing the state of the input as title and handling its changes. Instead, set the defaultValue to the current title, let the user do what they want, and then only enact the state change in the title field in the confirmEditedTitle method using the ref for the text input.

Then title would always be passed in as props, and the only job of this widget would be calling the updateTitle method when we're sure the user wants a new title.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent suggestion! Done! 👍

onKeyDown={this.handleTitleEditingKeyDown}
onBlur={this.confirmEditedTitle}
style={{ display: isLabelEditing ? 'block' : 'none' }}
/>
{!isLabelEditing && (
<h1 onDoubleClick={this.enableTitleEditing}>{this.state.title}</h1>
)}
{!isLabelEditing && (
<Button
type='button'
className='edit-label-toggle'
icon='codicon codicon-edit'
onClick={this.enableTitleEditing}
title='Edit view title'
aria-label='Edit view title'
rounded
aria-haspopup
/>
)}
</div>
<div className='core-options py-2'>
<Formik {...this.formConfig}>
{formik => (
Expand Down Expand Up @@ -327,4 +377,41 @@ export class OptionsWidget extends React.Component<OptionsWidgetProps, {}> {

protected handleResetAdvancedOptions: MouseEventHandler<HTMLButtonElement> | undefined = () => this.props.resetRenderOptions();

protected enableTitleEditing = () => this.doEnableTitleEditing();
protected doEnableTitleEditing(): void {
this.setState({ isTitleEditing: true, previousTitle: this.state.title });
}

protected disableTitleEditing = () => this.doDisableTitleEditing();
protected doDisableTitleEditing(): void {
this.setState({ isTitleEditing: false });
}

protected handleTitleEdit: ChangeEventHandler<HTMLInputElement> | undefined = () => this.doHandleTitleEdit();
protected doHandleTitleEdit(): void {
if (this.labelEditInput.current) {
this.setState({ title: this.labelEditInput.current?.value });
}
}

protected handleTitleEditingKeyDown: KeyboardEventHandler<HTMLInputElement> | undefined = event => this.doHandleTitleEditingKeyDown(event);
protected doHandleTitleEditingKeyDown(event: React.KeyboardEvent<HTMLInputElement>): void {
if (event.key === 'Enter' && this.labelEditInput.current) {
this.doConfirmEditedTitle();
} else if (event.key === 'Escape') {
if (this.state.previousTitle) {
this.setState({ title: this.state.previousTitle });
}
this.disableTitleEditing();
}
}

protected confirmEditedTitle: FocusEventHandler<HTMLInputElement> | undefined = () => this.doConfirmEditedTitle();
protected doConfirmEditedTitle(): void {
if (this.state.isTitleEditing && this.state.title) {
this.props.updateTitle(this.state.title);
this.disableTitleEditing();
}
}

}
7 changes: 7 additions & 0 deletions src/webview/memory-webview-view.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
logMessageType,
setOptionsType,
readMemoryType,
setTitleType,
setMemoryViewSettingsType,
resetMemoryViewSettingsType,
} from '../common/messaging';
Expand All @@ -39,6 +40,7 @@ import { PrimeReactProvider } from 'primereact/api';
import 'primeflex/primeflex.css';

export interface MemoryAppState extends MemoryState, MemoryDisplayConfiguration {
initialTitle: string;
decorations: Decoration[];
columns: ColumnStatus[];
}
Expand All @@ -59,6 +61,7 @@ class App extends React.Component<{}, MemoryAppState> {
columnContributionService.register(new AsciiColumn());
decorationService.register(variableDecorator);
this.state = {
initialTitle: 'Memory',
memory: undefined,
memoryReference: '',
offset: 0,
Expand All @@ -80,6 +83,7 @@ class App extends React.Component<{}, MemoryAppState> {
}
this.setState({ ...(config as MemoryDisplayConfiguration) });
});
messenger.onNotification(setTitleType, title => this.setState({ initialTitle: title }));
messenger.sendNotification(readyType, HOST_EXTENSION, undefined);
}

Expand All @@ -92,9 +96,11 @@ class App extends React.Component<{}, MemoryAppState> {
memoryReference={this.state.memoryReference}
offset={this.state.offset ?? 0}
count={this.state.count}
initialTitle={this.state.initialTitle}
updateMemoryArguments={this.updateMemoryState}
updateMemoryDisplayConfiguration={this.updateMemoryDisplayConfiguration}
resetMemoryDisplayConfiguration={this.resetMemoryDisplayConfiguration}
updateTitle={this.updateTitle}
refreshMemory={this.refreshMemory}
toggleColumn={this.toggleColumn}
fetchMemory={this.fetchMemory}
Expand All @@ -109,6 +115,7 @@ class App extends React.Component<{}, MemoryAppState> {
protected updateMemoryState = (newState: Partial<MemoryState>) => this.setState(prevState => ({ ...prevState, ...newState }));
protected updateMemoryDisplayConfiguration = (newState: Partial<MemoryDisplayConfiguration>) => this.setState(prevState => ({ ...prevState, ...newState }));
protected resetMemoryDisplayConfiguration = () => messenger.sendNotification(resetMemoryViewSettingsType, HOST_EXTENSION, undefined);
protected updateTitle = (title: string) => messenger.sendNotification(setTitleType, HOST_EXTENSION, title);

protected async setOptions(options?: Partial<DebugProtocol.ReadMemoryArguments>): Promise<void> {
messenger.sendRequest(logMessageType, HOST_EXTENSION, `Setting options: ${JSON.stringify(options)}`);
Expand Down
Loading