From eb2d8b32b127787aca539c402c646223f0e94e33 Mon Sep 17 00:00:00 2001 From: Martin Fleck Date: Tue, 2 Jul 2024 15:44:24 +0200 Subject: [PATCH] PR Feedback - Improve keyboard navigation to skip over non-existing groups - Copy should always use the focused group and not the selected one - Fix issue of losing focus when submitting a data edit - Support pasting values in data cells without going into edit mode - Improve styling in all themes -- Avoid orange focus outline and use same as tree for visibility -- Selected cells get their colors overwritten -- Slightly increase padding between two data groups --- media/index.css | 1 + media/memory-table.css | 22 ++- media/variable-decorations.css | 44 +++++ src/webview/columns/data-column.tsx | 95 +++++++---- src/webview/columns/table-group.tsx | 157 +++++++++++++----- src/webview/decorations/decoration-service.ts | 13 +- src/webview/utils/view-types.ts | 1 + src/webview/variables/variable-decorations.ts | 6 +- 8 files changed, 252 insertions(+), 87 deletions(-) create mode 100644 media/variable-decorations.css diff --git a/media/index.css b/media/index.css index fd38565..399e608 100644 --- a/media/index.css +++ b/media/index.css @@ -20,3 +20,4 @@ @import "./multi-select.css"; @import "./options-widget.css"; @import "./memory-table.css"; +@import "./variable-decorations.css"; diff --git a/media/memory-table.css b/media/memory-table.css index 7323f32..d476bd3 100644 --- a/media/memory-table.css +++ b/media/memory-table.css @@ -179,13 +179,17 @@ [role='group']:hover { border-bottom: 0px; - color: var(--vscode-list-activeSelectionForeground); outline: 1px solid var(--vscode-list-focusOutline); } [role='group'][data-group-selected='true'] { background: var(--vscode-list-activeSelectionBackground); color: var(--vscode-list-activeSelectionForeground); + outline: 1px solid var(--vscode-list-activeSelectionBackground); +} + +[role='group']:focus-visible, +[role='group']:focus { outline: 1px solid var(--vscode-list-focusOutline); } @@ -197,7 +201,7 @@ } [data-column="data"][role="group"], -[data-column="variables"][role="group"] { +[data-column="variables"][role="group"] { padding: 4px 1px; line-height: 23.5px; outline-offset: -1px; @@ -205,15 +209,15 @@ /* Data Column */ +[data-column="data"][role="group"] { + padding: 4px 2px; /* left-padding should match text-indent of data-edit */ +} /* == Data Edit == */ -[data-column="data"][role="group"]:last-child { - margin-right: 0px; -} - [data-column="data"][role="group"]:has(> .data-edit) { outline: 1px solid var(--vscode-inputOption-activeBorder); + background: transparent; padding-left: 0px; /* editing takes two more pixels cause the input field will cut off the characters otherwise. */ padding-right: 0px; /* editing takes two more pixels cause the input field will cut off the characters otherwise. */ } @@ -222,18 +226,18 @@ padding: 0; outline: 0; border: none; - text-indent: 1px; + text-indent: 2px; min-height: unset; height: 2ex; background: unset; margin: 0; + color: var(--vscode-editor-foreground) !important; } .data-edit:enabled:focus { outline: none; border: none; - color: var(--vscode-list-activeSelectionForeground); - text-indent: 1px; + text-indent: 2px; } .p-datatable .p-datatable-tbody > tr > td.p-highlight:has(>.selected) { diff --git a/media/variable-decorations.css b/media/variable-decorations.css new file mode 100644 index 0000000..884dbc8 --- /dev/null +++ b/media/variable-decorations.css @@ -0,0 +1,44 @@ +/******************************************************************************** + * Copyright (C) 2024 Ericsson and others. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License v. 2.0 which is available at + * http://www.eclipse.org/legal/epl-2.0. + * + * This Source Code may also be made available under the following Secondary + * Licenses when the conditions for such availability set forth in the Eclipse + * Public License v. 2.0 are satisfied: GNU General Public License, version 2 + * with the GNU Classpath Exception which is available at + * https://www.gnu.org/software/classpath/license.html. + * + * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 + ********************************************************************************/ + + /* Color wheel of variable decorations with 5 non-HC colors */ +.variable-0 { + color: var(--vscode-terminal-ansiBlue); +} + +.variable-1 { + color: var(--vscode-terminal-ansiGreen); +} + +.variable-2 { + color: var(--vscode-terminal-ansiBrightRed); +} + +.variable-3 { + color: var(--vscode-terminal-ansiYellow); +} + +.variable-4 { + color: var(--vscode-terminal-ansiMagenta); +} + +[role='group'][data-group-selected='true'] .variable-0, +[role='group'][data-group-selected='true'] .variable-1, +[role='group'][data-group-selected='true'] .variable-2, +[role='group'][data-group-selected='true'] .variable-3, +[role='group'][data-group-selected='true'] .variable-4 { + color: inherit; +} \ No newline at end of file diff --git a/src/webview/columns/data-column.tsx b/src/webview/columns/data-column.tsx index 29300f6..c4ef42c 100644 --- a/src/webview/columns/data-column.tsx +++ b/src/webview/columns/data-column.tsx @@ -16,6 +16,7 @@ import { ColumnPassThroughOptions } from 'primereact/column'; import { InputText } from 'primereact/inputtext'; +import { classNames } from 'primereact/utils'; import * as React from 'react'; import { HOST_EXTENSION } from 'vscode-messenger-common'; import { Memory } from '../../common/memory'; @@ -31,10 +32,10 @@ import { AddressColumn } from './address-column'; import { ColumnContribution, ColumnRenderProps } from './column-contribution-service'; import { findGroup, - getDefaultSearchContext, getGroupPosition, groupAttributes, - GroupPosition, handleGroupNavigation, + GroupPosition, + handleGroupNavigation, handleGroupSelection, SelectionProps } from './table-group'; @@ -97,7 +98,11 @@ export interface EditableDataColumnRowProps { config: ColumnRenderProps; } -export class EditableDataColumnRow extends React.Component { +export interface EditableDataColumnRowState { + position?: GroupPosition; +} + +export class EditableDataColumnRow extends React.Component { protected inputText = React.createRef(); protected toDisposeOnUnmount?: Disposable; @@ -112,6 +117,14 @@ export class EditableDataColumnRow extends React.Component, prevState: Readonly): void { + const editingPosition = prevState?.position; + if (editingPosition && !this.state.position) { + // we went out of editing mode --> restore focus + setTimeout(() => findGroup(editingPosition)?.focus()); + } + } + protected renderGroups(): React.ReactNode { const { row, config } = this.props; const groups = []; @@ -192,9 +205,10 @@ export class EditableDataColumnRow extends React.Component byte.toString(16).padStart(2, '0')).join(''); } - protected onBlur: React.FocusEventHandler = event => { - this.submitChanges(event); + protected onBlur: React.FocusEventHandler = _event => { + this.submitChanges(); }; - protected onKeyDown: React.KeyboardEventHandler = event => { + protected onKeyDown: React.KeyboardEventHandler = async event => { switch (event.key) { case ' ': { this.setGroupEdit(event); break; } + case 'v': { + if (event.ctrlKey) { + // paste clipboard text and submit as change + const range = getAddressRange(event.currentTarget); + if (range) { + const text = await navigator.clipboard.readText(); + if (text.length > 0) { + this.submitChanges(text, range); + } + } + } + break; + } } handleGroupNavigation(event); handleGroupSelection(event, this.selectionProps); @@ -255,11 +283,11 @@ export class EditableDataColumnRow extends React.Component = event => { switch (event.key) { case 'Escape': { - this.disableEdit(event); + this.disableEdit(); break; } case 'Enter': { - this.submitChanges(event); + this.submitChanges(); break; } } @@ -276,6 +304,7 @@ export class EditableDataColumnRow extends React.Component findGroup(position, context)?.focus()); - } + this.setState({ position: undefined }); } } - protected async submitChanges(event: React.BaseSyntheticEvent): Promise { - if (!this.inputText.current || !DataColumnSelection.is(this.props.config.selection) || !this.props.config.selection.editingRange) { return; } + protected async submitChanges(data = this.inputText.current?.value, range?: BigIntMemoryRange): Promise { + if (!data || !DataColumnSelection.is(this.props.config.selection)) { return; } - const originalData = this.createEditingGroupDefaultValue(this.props.config.selection.editingRange); - if (originalData !== this.inputText.current.value) { - const newMemoryValue = this.processData(this.inputText.current.value, this.props.config.selection.editingRange); + const editingRange = range ?? this.props.config.selection.editingRange; + if (!editingRange) { + return; + } + const originalData = this.createEditingGroupDefaultValue(editingRange); + if (originalData !== data) { + const newMemoryValue = this.processData(data, editingRange); const converted = Buffer.from(newMemoryValue, 'hex').toString('base64'); await messenger.sendRequest(writeMemoryType, HOST_EXTENSION, { - memoryReference: toHexStringWithRadixMarker(this.props.config.selection.editingRange.startAddress), + memoryReference: toHexStringWithRadixMarker(editingRange.startAddress), data: converted }).catch(() => { }); } - this.disableEdit(event); + this.disableEdit(); } protected processData(data: string, editedRange: BigIntMemoryRange): string { @@ -341,10 +366,8 @@ export class EditableDataColumnRow extends React.Component(position: GroupPosition, element?: Element | null): T | undefined { +export function findGroup(position: Omit, element?: Element | null): T | undefined { const context = element ?? document.documentElement; // eslint-disable-next-line max-len const group = context.querySelector(`[${GroupPosition.Attributes.ColumnIndex}="${position.columnIndex}"][${GroupPosition.Attributes.RowIndex}="${position.rowIndex}"][${GroupPosition.Attributes.GroupIndex}="${position.groupIndex}"]`); @@ -98,47 +99,23 @@ export function findGroup(position: GroupPosition, element?: } export function handleGroupNavigation(event: React.KeyboardEvent): boolean { + const currentGroup = event.target as HTMLElement; + + let targetGroup: HTMLElement | null = null; + switch (event.key) { - case 'ArrowRight': { - const rightGroup = findRightGroup(event.currentTarget); - if (rightGroup) { - rightGroup.focus(); - event.preventDefault(); - event.stopPropagation(); - return true; - } + case 'ArrowRight': + targetGroup = getNextGroup(currentGroup); break; - } - case 'ArrowLeft': { - const leftGroup = findLeftGroup(event.currentTarget); - if (leftGroup) { - leftGroup?.focus?.(); - event.preventDefault(); - event.stopPropagation(); - return true; - } + case 'ArrowLeft': + targetGroup = getPreviousGroup(currentGroup); break; - } - case 'ArrowUp': { - const upGroup = findUpGroup(event.currentTarget); - if (upGroup) { - upGroup?.focus?.(); - event.preventDefault(); - event.stopPropagation(); - return true; - } + case 'ArrowDown': + targetGroup = getGroupInNextRow(currentGroup); break; - } - case 'ArrowDown': { - const downGroup = findDownGroup(event.currentTarget); - if (downGroup) { - downGroup?.focus?.(); - event.preventDefault(); - event.stopPropagation(); - return true; - } + case 'ArrowUp': + targetGroup = getGroupInPreviousRow(currentGroup); break; - } case 'c': { if (event.ctrlKey) { handleCopy(event); @@ -152,6 +129,10 @@ export function handleGroupNavigation(event: React.Keyboa break; } } + if (targetGroup) { + event.preventDefault(); + targetGroup.focus(); + } return false; } @@ -225,6 +206,7 @@ export function createDefaultSelection(event: React.MouseEvent | Re function handleCopy(event: React.ClipboardEvent | React.KeyboardEvent): void { event.preventDefault(); + event.stopPropagation(); const textSelection = window.getSelection()?.toString(); if (textSelection) { navigator.clipboard.writeText(textSelection); @@ -267,3 +249,104 @@ export function toggleSelection(event: React.MouseEvent 0) { return groups[0] as HTMLElement; } + const nextCell = cell.nextElementSibling; + return nextCell ? findFirstGroup(nextCell) : null; +} + +function findLastGroup(cell: Element): HTMLElement | null { + const groups = cell.querySelectorAll('[role="group"]'); + if (groups.length > 0) { return groups[groups.length - 1] as HTMLElement; } + const prevCell = cell.previousElementSibling; + return prevCell ? findLastGroup(prevCell) : null; +} + +function getNextGroupInNextCells(cell: Element | null): HTMLElement | null { + if (!cell) { return null; } + const nextCell = cell.nextElementSibling; + if (nextCell) { + return findFirstGroup(nextCell); + } + const nextRow = cell.closest('tr')?.nextElementSibling?.firstElementChild; + if (nextRow) { + return findFirstGroup(nextRow); + } + return null; +} + +function getPreviousGroupInPreviousCells(cell: Element | null): HTMLElement | null { + if (!cell) { return null; } + const prevCell = cell.previousElementSibling; + if (prevCell) { + return findLastGroup(prevCell); + } + const prevRow = cell.closest('tr')?.previousElementSibling?.lastElementChild; + if (prevRow) { + return findLastGroup(prevRow); + } + return null; +} + +function findGroupInSamePosition(currentCell: Element | null, targetCell: Element | null): HTMLElement | null { + if (!currentCell || !targetCell) { return null; } + const groups = Array.from(targetCell.querySelectorAll('[role="group"]')); + const currentGroups = Array.from(currentCell.querySelectorAll('[role="group"]')); + const currentIndex = currentGroups.indexOf(document.activeElement as HTMLElement); + return groups[currentIndex] as HTMLElement || groups[0] as HTMLElement || null; +} diff --git a/src/webview/decorations/decoration-service.ts b/src/webview/decorations/decoration-service.ts index b3335d3..101464b 100644 --- a/src/webview/decorations/decoration-service.ts +++ b/src/webview/decorations/decoration-service.ts @@ -73,16 +73,23 @@ class DecorationService { if (index === terminiInOrder.length - 1) { return; } const decoration: Decoration = { range: { startAddress: terminus, endAddress: terminiInOrder[index + 1] }, - style: {} + style: {}, + classNames: [] }; decorations.push(decoration); currentSubDecorations.forEach((subDecoration, subDecorationIndex) => { switch (determineRelationship(terminus, subDecoration?.range)) { - case RangeRelationship.Within: Object.assign(decoration.style, subDecoration?.style); + case RangeRelationship.Within: { + Object.assign(decoration.style, subDecoration?.style); + Object.assign(decoration.classNames, subDecoration?.classNames); + } break; case RangeRelationship.Past: { const newSubDecoration = currentSubDecorations[subDecorationIndex] = contributions[subDecorationIndex].next().value; - if (determineRelationship(terminus, newSubDecoration.range) === RangeRelationship.Within) { Object.assign(decoration.style, newSubDecoration.style); } + if (determineRelationship(terminus, newSubDecoration.range) === RangeRelationship.Within) { + Object.assign(decoration.style, newSubDecoration.style); + Object.assign(decoration.classNames, subDecoration?.classNames); + } } } }); diff --git a/src/webview/utils/view-types.ts b/src/webview/utils/view-types.ts index 85e7add..27aab28 100644 --- a/src/webview/utils/view-types.ts +++ b/src/webview/utils/view-types.ts @@ -38,6 +38,7 @@ export function dispose(disposable: { dispose(): unknown }): void { export interface Decoration { range: BigIntMemoryRange; style: React.CSSProperties; + classNames: string[]; } export function areDecorationsEqual(one: Decoration, other: Decoration): boolean { diff --git a/src/webview/variables/variable-decorations.ts b/src/webview/variables/variable-decorations.ts index 20304a4..b15cfcf 100644 --- a/src/webview/variables/variable-decorations.ts +++ b/src/webview/variables/variable-decorations.ts @@ -33,7 +33,7 @@ import { messenger } from '../view-messenger'; const NON_HC_COLORS = [ 'var(--vscode-terminal-ansiBlue)', 'var(--vscode-terminal-ansiGreen)', - 'var(--vscode-terminal-ansiRed)', + 'var(--vscode-terminal-ansiBrightRed)', 'var(--vscode-terminal-ansiYellow)', 'var(--vscode-terminal-ansiMagenta)', ] as const; @@ -132,12 +132,14 @@ export class VariableDecorator implements ColumnContribution, Decorator { let colorIndex = 0; for (const variable of this.currentVariables ?? []) { if (variable.endAddress) { + const idx = colorIndex++ % 5; decorations.push({ range: { startAddress: variable.startAddress, endAddress: variable.endAddress }, - style: { color: NON_HC_COLORS[colorIndex++ % 5] } + style: {}, + classNames: ['variable-' + idx] }); } }