Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
239 changes: 212 additions & 27 deletions packages/markers/src/browser/problem/problem-contribution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,24 @@ import {
FrontendApplication, FrontendApplicationContribution, CompositeTreeNode, SelectableTreeNode, Widget, codicon,
TreeNode, TreeSelection
} from '@theia/core/lib/browser';
import { MessageService } from '@theia/core/lib/common/message-service';
import { StatusBar, StatusBarAlignment } from '@theia/core/lib/browser/status-bar/status-bar';
import { AbstractViewContribution } from '@theia/core/lib/browser/shell/view-contribution';
import { PROBLEM_KIND, ProblemMarker } from '../../common/problem-marker';
import { ProblemManager, ProblemStat } from './problem-manager';
import { ProblemWidget, PROBLEMS_WIDGET_ID } from './problem-widget';
import { ProblemTreeModel } from './problem-tree-model';
import { MarkerNode } from '../marker-tree';
import { MarkerNode, MarkerInfoNode } from '../marker-tree';
import { MenuPath, MenuModelRegistry } from '@theia/core/lib/common/menu';
import { Command, CommandRegistry } from '@theia/core/lib/common/command';
import { TabBarToolbarContribution, TabBarToolbarRegistry } from '@theia/core/lib/browser/shell/tab-bar-toolbar';
import { KeybindingContribution, KeybindingRegistry } from '@theia/core/lib/browser/keybinding';
import { LabelProvider } from '@theia/core/lib/browser/label-provider';
import { ContextKeyService } from '@theia/core/lib/browser/context-key-service';
import { ProblemSelection } from './problem-selection';
import { nls } from '@theia/core/lib/common/nls';
import { FileDialogService } from '@theia/filesystem/lib/browser';
import { FileService } from '@theia/filesystem/lib/browser/file-service';

export const PROBLEMS_CONTEXT_MENU: MenuPath = [PROBLEM_KIND];

Expand All @@ -48,15 +54,32 @@ export namespace ProblemsCommands {
id: 'problems.collapse.all.toolbar',
iconClass: codicon('collapse-all')
};
export const COPY: Command = {
id: 'problems.copy'
};
export const COPY_MESSAGE: Command = {
export const COPY = Command.toDefaultLocalizedCommand({
id: 'problems.copy',
category: 'Problems',
label: 'Copy'
});
export const COPY_MESSAGE = Command.toDefaultLocalizedCommand({
id: 'problems.copy.message',
};
export const SELECT_ALL: Command = {
id: 'problems.select.all'
};
category: 'Problems',
label: 'Copy Message'
});
export const COPY_AS_TEXT = Command.toLocalizedCommand({
id: 'problems.copy.as.text',
category: 'Problems',
label: 'Copy as Text'
}, 'theia/markers/copyAsText', nls.getDefaultKey('Problems'));
export const SELECT_ALL = Command.toDefaultLocalizedCommand({
id: 'problems.select.all',
category: 'Problems',
label: 'Select All'
});
export const EXPORT = Command.toLocalizedCommand({
id: 'problems.export',
category: 'Problems',
label: 'Export',
iconClass: codicon('arrow-circle-up', true)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
iconClass: codicon('arrow-circle-up', true)
iconClass: codicon('arrow-circle-up')

Does not need to be declared as action item, since i breaks if it s rendered in a (context) menu, e.g.
Image

}, 'theia/markers/export', nls.getDefaultKey('Problems'));
export const CLEAR_ALL = Command.toLocalizedCommand({
id: 'problems.clear.all',
category: 'Problems',
Expand All @@ -66,10 +89,15 @@ export namespace ProblemsCommands {
}

@injectable()
export class ProblemContribution extends AbstractViewContribution<ProblemWidget> implements FrontendApplicationContribution, TabBarToolbarContribution {
export class ProblemContribution extends AbstractViewContribution<ProblemWidget> implements FrontendApplicationContribution, TabBarToolbarContribution, KeybindingContribution {

@inject(ProblemManager) protected readonly problemManager: ProblemManager;
@inject(StatusBar) protected readonly statusBar: StatusBar;
@inject(FileDialogService) protected readonly fileDialogService: FileDialogService;
@inject(FileService) protected readonly fileService: FileService;
@inject(LabelProvider) protected readonly labelProvider: LabelProvider;
@inject(ContextKeyService) protected readonly contextKeyService: ContextKeyService;
@inject(MessageService) protected readonly messageService: MessageService;

constructor() {
super({
Expand All @@ -86,6 +114,12 @@ export class ProblemContribution extends AbstractViewContribution<ProblemWidget>
onStart(app: FrontendApplication): void {
this.updateStatusBarElement();
this.problemManager.onDidChangeMarkers(this.updateStatusBarElement);
const problemsFocusKey = this.contextKeyService.createKey<boolean>('problemsFocus', false);
const updateFocusKey = () => {
problemsFocusKey.set(this.shell.activeWidget instanceof ProblemWidget);
};
updateFocusKey();
this.shell.onDidChangeActiveWidget(updateFocusKey);
}

async initializeLayout(app: FrontendApplication): Promise<void> {
Expand Down Expand Up @@ -165,11 +199,28 @@ export class ProblemContribution extends AbstractViewContribution<ProblemWidget>
}
})
});
commands.registerCommand(ProblemsCommands.COPY_AS_TEXT, {
isEnabled: () => this.withWidget(undefined, () => true),
isVisible: () => this.withWidget(undefined, () => true),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

isVisible is always true here (and for EXPORT at line 219), so both entries always show in the context menu even with an empty selection, where execute then no-ops. COPY/COPY_MESSAGE instead hide when nothing is selected (getSelectedMarkerNodes(widget).length > 0). Please align for consistency.

execute: () => this.withWidget(undefined, widget => {
const selectedSet = new Set(widget.model.selectedNodes);
if (selectedSet.size > 0) {
this.copyAsText(widget, widget.model.root, selectedSet);
}
})
});
commands.registerCommand(ProblemsCommands.SELECT_ALL, {
isEnabled: () => this.withWidget(undefined, () => true),
isVisible: () => this.withWidget(undefined, () => true),
execute: () => this.withWidget(undefined, widget => this.selectAllProblems(widget))
});
commands.registerCommand(ProblemsCommands.EXPORT, {
isEnabled: () => this.withWidget(undefined, () => true),
isVisible: () => this.withWidget(undefined, () => true),
execute: async () => {
await this.withWidget(undefined, async widget => await this.exportProblems(widget));
}
});
commands.registerCommand(ProblemsCommands.CLEAR_ALL, {
isEnabled: widget => this.withWidget(widget, () => true),
isVisible: widget => this.withWidget(widget, () => true),
Expand All @@ -189,6 +240,14 @@ export class ProblemContribution extends AbstractViewContribution<ProblemWidget>
label: nls.localizeByDefault('Copy Message'),
order: '1'
});
menus.registerMenuAction(ProblemsMenu.CLIPBOARD, {
commandId: ProblemsCommands.COPY_AS_TEXT.id,
order: '2'
});
menus.registerMenuAction(ProblemsMenu.CLIPBOARD, {
commandId: ProblemsCommands.EXPORT.id,
order: '3'
});
menus.registerMenuAction(ProblemsMenu.PROBLEMS, {
commandId: ProblemsCommands.COLLAPSE_ALL.id,
label: nls.localizeByDefault('Collapse All'),
Expand Down Expand Up @@ -216,6 +275,19 @@ export class ProblemContribution extends AbstractViewContribution<ProblemWidget>
});
}

override registerKeybindings(keybindings: KeybindingRegistry): void {
keybindings.registerKeybinding({
command: ProblemsCommands.COPY.id,
keybinding: 'ctrlcmd+c',
when: 'problemsFocus'
});
keybindings.registerKeybinding({
command: ProblemsCommands.SELECT_ALL.id,
keybinding: 'ctrlcmd+a',
when: 'problemsFocus'
});
}

protected async collapseAllProblems(): Promise<void> {
const { model } = await this.widget;
const root = model.root as CompositeTreeNode;
Expand All @@ -226,7 +298,7 @@ export class ProblemContribution extends AbstractViewContribution<ProblemWidget>
}
}

protected async selectAllProblems(widget: ProblemWidget): Promise<void> {
protected selectAllProblems(widget: ProblemWidget): void {
const { model } = widget;
const root = model.root as CompositeTreeNode;
if (root) {
Expand Down Expand Up @@ -264,22 +336,11 @@ export class ProblemContribution extends AbstractViewContribution<ProblemWidget>

protected copy(selections: ProblemSelection | ProblemSelection[]): void {
const selectionsArray = Array.isArray(selections) ? selections : [selections];
const serializedProblems = selectionsArray.map(selection => {
const marker = selection.marker as ProblemMarker;
return {
resource: marker.uri,
owner: marker.owner,
code: marker.data.code,
severity: marker.data.severity,
message: marker.data.message,
source: marker.data.source,
startLineNumber: marker.data.range.start.line,
startColumn: marker.data.range.start.character,
endLineNumber: marker.data.range.end.line,
endColumn: marker.data.range.end.character
};
});
this.addToClipboard(JSON.stringify(serializedProblems, undefined, '\t'));
const serializedProblems = selectionsArray.map(selection => this.serializeMarker(selection.marker as ProblemMarker));
const output = selectionsArray.length === 1
? JSON.stringify(serializedProblems[0], undefined, '\t')
Comment on lines +340 to +341

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This changes the existing Copy output: a single selection now serializes a bare object instead of a one-element array. I think we changed that previously to array right? So I'd be in favour of keeping it this way now.

: JSON.stringify(serializedProblems, undefined, '\t');
this.addToClipboard(output);
}

protected copyMessage(selections: ProblemSelection | ProblemSelection[]): void {
Expand All @@ -291,6 +352,130 @@ export class ProblemContribution extends AbstractViewContribution<ProblemWidget>
this.addToClipboard(messages.join('\n'));
}

protected serializeMarker(marker: ProblemMarker): object {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

serializeMarker returns object, which discards all type information for callers (copy, exportProblems). Please define a proper interface for the serialized shape and return that instead — it documents the export/copy format and, combined with the severity fix above, keeps severity typed as DiagnosticSeverity end-to-end:
Something like:

export interface SerializedProblemMarker {
   resource: string;
   ...
}

...

protected serializeMarker(marker: ProblemMarker): SerializedProblemMarker

return {
resource: marker.uri,
owner: marker.owner,
code: marker.data.code,
severity: marker.data.severity,
message: marker.data.message,
source: marker.data.source,
startLineNumber: marker.data.range.start.line,
startColumn: marker.data.range.start.character,
endLineNumber: marker.data.range.end.line,
endColumn: marker.data.range.end.character
};
}

protected copyAsText(widget: ProblemWidget, root: TreeNode | undefined, selectedSet: Set<TreeNode>): void {
const lines: string[] = [];
if (root) {
this.collectSelectedLinesInOrder(widget, root, selectedSet, lines);
}
this.addToClipboard(lines.join('\n'));
}

protected collectSelectedLinesInOrder(widget: ProblemWidget, node: TreeNode, selectedSet: Set<TreeNode>, lines: string[]): void {
if (selectedSet.has(node)) {
if (MarkerInfoNode.is(node)) {
lines.push(this.formatMarkerInfoNode(widget, node));
} else if (MarkerNode.is(node)) {
lines.push(this.formatMarkerNode(node));
}
}
if (CompositeTreeNode.is(node) && node.children) {
for (const child of node.children) {
this.collectSelectedLinesInOrder(widget, child, selectedSet, lines);
}
}
}

protected formatMarkerInfoNode(widget: ProblemWidget, node: MarkerInfoNode): string {
const name = widget.toNodeName(node);
const description = widget.toNodeDescription(node);
return description ? `${name} ${description}` : name;
}

protected formatMarkerNode(node: MarkerNode): string {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This re-implements the message/source/code/location formatting already done in ProblemWidget.decorateMarkerNode, and the two have already drifted (severity info here vs information there). Consider extracting a shared helper so the copied text and the rendered row can't diverge.

const marker = node.marker as ProblemMarker;
const severity = this.getSeverityLabel(marker.data.severity);
const line = marker.data.range.start.line + 1;
const column = marker.data.range.start.character + 1;
const location = nls.localizeByDefault('Ln {0}, Col {1}', line, column);
const source = marker.data.source ? `${marker.data.source}` : '';
const code = marker.data.code ? `(${marker.data.code})` : '';
const sourceCode = [source, code].filter(s => s).join(' ');
const suffix = sourceCode ? ` ${sourceCode}` : '';
return `${severity}: ${marker.data.message}${suffix} [${location}]`;
}

protected getSeverityLabel(severity: number | undefined): string {
switch (severity) {
case 1: return 'error';
case 2: return 'warning';
case 3: return 'info';
default: return 'hint';

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These severity labels are hardcoded numbers plus untranslated strings. marker.data.severity is a DiagnosticSeverity (already imported in problem-widget.tsx), so please switch on DiagnosticSeverity.Error/.Warning/.Information/.Hint instead of the magic 1/2/3, and localize the returned labels (e.g. nls.localizeByDefault('Error')).

}
}

protected async exportProblems(widget: ProblemWidget): Promise<void> {
const selectedNodes = widget.model.selectedNodes;

if (selectedNodes.length === 0) {
return;
}

const markerNodes = this.collectMarkerNodes(selectedNodes);

if (markerNodes.length === 0) {
return;
}

const filePath = await this.fileDialogService.showSaveDialog({
title: 'Export Problems',
filters: { 'JSON Files': ['json'] },
saveLabel: 'Export'
Comment on lines +435 to +437

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These string are user-facing but not localized. Please make sure to use nls.localizeByDefault/nls.localize for those.

});

if (!filePath) {
return;
}

try {
const serializedProblems = markerNodes.map(node => this.serializeMarker(node.marker as ProblemMarker));
const content = JSON.stringify(serializedProblems, undefined, '\t');
await this.fileService.write(filePath, content);
this.messageService.info(nls.localize('theia/markers/exportSuccess', 'Problems exported successfully.'));
} catch (error) {
const errorMessage = error instanceof Error ? error.message : String(error);
this.messageService.error(errorMessage);
}
}

protected collectMarkerNodes(nodes: readonly TreeNode[]): MarkerNode[] {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

collectMarkerNodes and collectMarkerNodesRecursive are split into two methods for what is really one recursive walk. You can fold them into a single recursive function (seed the Set and recurse in one place), since a MarkerNode never has MarkerNode children anyway.

More generally, this PR now has three separate walks over the same tree: getSelectedMarkerNodes (flat), collectSelectedLinesInOrder, and this pair. It would be worth consolidating the "collect selected marker nodes" logic into one helper that the copy, copy-as-text, and export paths all reuse.

const seen = new Set<MarkerNode>();
for (const node of nodes) {
if (MarkerNode.is(node)) {
seen.add(node);
} else if (CompositeTreeNode.is(node)) {
this.collectMarkerNodesRecursive(node, seen);
}
}
return Array.from(seen);
}

protected collectMarkerNodesRecursive(node: CompositeTreeNode, seen: Set<MarkerNode>): void {
if (node.children) {
for (const child of node.children) {
if (MarkerNode.is(child)) {
seen.add(child);
} else if (CompositeTreeNode.is(child)) {
this.collectMarkerNodesRecursive(child, seen);
}
}
}
}

protected withWidget<T>(widget: Widget | undefined = this.tryGetWidget(), cb: (problems: ProblemWidget) => T): T | false {
if (widget instanceof ProblemWidget && widget.id === PROBLEMS_WIDGET_ID) {
return cb(widget);
Expand Down
8 changes: 8 additions & 0 deletions packages/markers/src/browser/problem/problem-widget.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,14 @@ export class ProblemWidget extends TreeWidget {
return <ProblemMarkerRemoveButton model={this.model} node={node} />;
}

override toNodeName(node: TreeNode): string {
return super.toNodeName(node);
}

override toNodeDescription(node: TreeNode): string {
return super.toNodeDescription(node);
}
Comment on lines +178 to +184

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These overrides only call super to make toNodeName/toNodeDescription public so the contribution can reach them. Widening a widget's protected methods just to call them from outside is not good practice.

You can avoid this though. Both base methods just delegate to the LabelProvider (getName / getLongName), which ProblemContribution already injects. So compute the text directly in the contribution and drop both overrides:

protected formatMarkerInfoNode(node: MarkerInfoNode): string {
   const name = this.labelProvider.getName(node);
   const description = this.labelProvider.getLongName(node);
   return description ? `${name} ${description}` : name;
}


protected decorateMarkerNode(node: MarkerNode): React.ReactNode {
if (ProblemMarker.is(node.marker)) {
let severityClass: string = '';
Expand Down
Loading