add copy as text, export, and keybinds#17717
Conversation
Signed-off-by: Roger Gu <r-gu@ti.com>
| id: 'problems.export', | ||
| category: 'Problems', | ||
| label: 'Export', | ||
| iconClass: codicon('arrow-circle-up', true) |
| title: 'Export Problems', | ||
| filters: { 'JSON Files': ['json'] }, | ||
| saveLabel: 'Export' |
There was a problem hiding this comment.
These string are user-facing but not localized. Please make sure to use nls.localizeByDefault/nls.localize for those.
| const output = selectionsArray.length === 1 | ||
| ? JSON.stringify(serializedProblems[0], undefined, '\t') |
There was a problem hiding this comment.
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.
| }); | ||
| commands.registerCommand(ProblemsCommands.COPY_AS_TEXT, { | ||
| isEnabled: () => this.withWidget(undefined, () => true), | ||
| isVisible: () => this.withWidget(undefined, () => true), |
There was a problem hiding this comment.
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.
| return description ? `${name} ${description}` : name; | ||
| } | ||
|
|
||
| protected formatMarkerNode(node: MarkerNode): string { |
There was a problem hiding this comment.
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.
| case 1: return 'error'; | ||
| case 2: return 'warning'; | ||
| case 3: return 'info'; | ||
| default: return 'hint'; |
There was a problem hiding this comment.
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')).
| this.addToClipboard(messages.join('\n')); | ||
| } | ||
|
|
||
| protected serializeMarker(marker: ProblemMarker): object { |
There was a problem hiding this comment.
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| override toNodeName(node: TreeNode): string { | ||
| return super.toNodeName(node); | ||
| } | ||
|
|
||
| override toNodeDescription(node: TreeNode): string { | ||
| return super.toNodeDescription(node); | ||
| } |
There was a problem hiding this comment.
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 collectMarkerNodes(nodes: readonly TreeNode[]): MarkerNode[] { |
There was a problem hiding this comment.
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.
What it does
Implements #17706.
How to test
Follow-ups
Breaking changes
Attribution
Review checklist
nlsservice (for details, please see the Internationalization/Localization section in the Coding Guidelines)Reminder for reviewers