themable debug icons #111183

aeschli · 2020-11-23T15:38:04Z

no more codicon references in code
define debug specific icon name for reused icons

@isidorn You might want to look at the names and descriptions given in https://github.com/microsoft/vscode/pull/111183/files#diff-66eeaa212162fdffb80b665f45fa68fd3f66c27504635227161fb9efa41edf27

I double/trippled checked that I didn't introduce any regressions, but from experience there are always one or two...

isidorn · 2020-11-23T16:14:08Z

src/vs/workbench/contrib/debug/browser/debugActions.ts

@@ -330,7 +331,7 @@ export class AddWatchExpressionAction extends AbstractDebugAction {
 	static readonly LABEL = nls.localize('addWatchExpression', "Add Expression");

 	constructor(id: string, label: string, @IDebugService debugService: IDebugService, @IKeybindingService keybindingService: IKeybindingService) {
-		super(id, label, 'debug-action codicon-add', debugService, keybindingService);
+		super(id, label, 'debug-action ' + icons.watchExpressionsAdd.classNames, debugService, keybindingService);


We previously had just one add icon which was the plus add, now it would be possible for somebody to customise to have multiple different icons for different adds. Does not really matter I guess

It's up to you. If it is essential that the same icon is used, we can have a single add icon. Makes things simpler, for sure.

Yeah I would go with a single add icon.

isidorn · 2020-11-23T16:14:39Z

src/vs/workbench/contrib/debug/browser/debugActions.ts

@@ -350,7 +351,7 @@ export class RemoveAllWatchExpressionsAction extends AbstractDebugAction {
 	static readonly LABEL = nls.localize('removeAllWatchExpressions', "Remove All Expressions");

 	constructor(id: string, label: string, @IDebugService debugService: IDebugService, @IKeybindingService keybindingService: IKeybindingService) {
-		super(id, label, 'debug-action codicon-close-all', debugService, keybindingService);
+		super(id, label, 'debug-action ' + icons.watchExpressionsRemoveAll.classNames, debugService, keybindingService);


Same for close-all, now you are adding a remove-all for each case. Which is fine I think

isidorn · 2020-11-23T16:15:45Z

src/vs/workbench/contrib/debug/browser/debugIcons.ts

+export const debugStackframe = registerIcon('debug-stackframe', Codicon.debugStackframe, localize('debugStackframe', 'Icon for a stackframe.'));
+export const debugStackframeFocused = registerIcon('debug-stackframe-focused', Codicon.debugStackframeFocused, localize('debugStackframeFocused', 'Icon for a focused stackframe.'));
+
+export const debugGripper = registerIcon('debug-gripper', Codicon.gripper, localize('debugGripper', 'Icon for the debug bar gripper.'));


Which icon is this?

Aaaaa now I figured out. The one from the quick pick. This also does not feel debug specific and should be a general workbench icon

It's the ' grip handle' on the toolbar. https://code.visualstudio.com/api/references/icons-in-labels
I would say we should allow themes to customize that just for the debug toolbar as it is quire special.

Ok, makes sense.

isidorn · 2020-11-23T16:16:29Z

src/vs/workbench/contrib/debug/browser/debugIcons.ts

+
+export const debugStackframeActive = registerIcon('debug-stackframe-active', Codicon.debugStackframeActive, localize('debugStackframeActive', 'Icon for an active stackframe.'));
+export const debugStackframeDot = registerIcon('debug-stackframe-dot', Codicon.debugStackframeDot, localize('debugStackframeDot', 'Icon for a stackframe dot.'));
+export const debugStackframe = registerIcon('debug-stackframe', Codicon.debugStackframe, localize('debugStackframe', 'Icon for a stackframe.'));


Mention in the descriptoin: that Stackframe and StackframeFocused are shown in the Editor glyph margin.

isidorn · 2020-11-23T16:17:08Z

src/vs/workbench/contrib/debug/browser/debugIcons.ts

+export const debugBreakpoint = registerIcon('debug-breakpoint', Codicon.debugBreakpoint, localize('debugBreakpoint', 'Icon for breakpoints.'));
+export const debugBreakpointDisabled = registerIcon('debug-breakpoint-disabled', Codicon.debugBreakpointDisabled, localize('debugBreakpointDisabled', 'Icon for disabled breakpoints.'));
+export const debugBreakpointUnverified = registerIcon('debug-breakpoint-unverified', Codicon.debugBreakpointUnverified, localize('debugBreakpointUnverified', 'Icon for unverified breakpoints.'));
+export const debugBreakpointHint = registerIcon('debug-hint', Codicon.debugHint, localize('debugBreakpointHint', 'Icon for breakpoint hints.'));


Add in description: "shown on hover in editor glyph margin"

isidorn · 2020-11-23T16:19:10Z

src/vs/workbench/contrib/debug/browser/debugIcons.ts

+export const watchExpressionsAddFuncBreakpoint = registerIcon('watch-expressions-add-function-breakpoint', Codicon.add, localize('watchExpressionsAddFuncBreakpoint', 'Icon for the add function breakpoint action in the watch view.'));
+
+export const breakpointsRemoveAll = registerIcon('breakpoints-remove-all', Codicon.closeAll, localize('breakpointsRemoveAll', 'Icon for the remove all action in the breakpoints view.'));
+export const breakpointsActivate = registerIcon('breakpoints-activate', Codicon.activateBreakpoints, localize('breakpointsActivate', 'Icon for the activatel action in the breakpoints view.'));


activatel is a typo

isidorn · 2020-11-23T16:19:54Z

src/vs/workbench/contrib/debug/browser/debugIcons.ts

+
+export const debugConsoleEvaluationInput = registerIcon('debug-console-evaluation-input', Codicon.arrowSmallRight, localize('debugConsoleEvaluationInput', 'Icon for the debug evaluation input marker.'));
+export const debugConsoleEvaluationPrompt = registerIcon('debug-console-evaluation-prompt', Codicon.chevronRight, localize('debugConsoleEvaluationPrompt', 'Icon for the debug evaluation prompt.'));
+export const debugExceptionWidgetClose = registerIcon('debug-exception-widget-close', Codicon.close, localize('debugExceptionWidgetClose', 'Icon for the close action of the debug exception widget.'));


Yeah not a fan of having multiple close action. I think these should be aligned across the workbench across our widgets. x should look everywhere the same

Makes sense. I started a list of common icons in the iconRegistry

isidorn · 2020-11-23T16:23:38Z

@aeschli thanks a lot for cleaning this up, really appreciate it.
I did a review of your changes and they look great - I left some minor comments in the PR.
I would like that we merge this as soon as possible so we catch breakages in Insiders -> thus approved

I did not test this. Let me know if you would be more comfortable if I test it out and I can.

externalize debug icons

0223a22

aeschli requested a review from isidorn November 23, 2020 15:38

aeschli added this to the November 2020 milestone Nov 23, 2020

aeschli self-assigned this Nov 23, 2020

isidorn reviewed Nov 23, 2020

View reviewed changes

isidorn approved these changes Nov 23, 2020

View reviewed changes

aeschli added 4 commits November 24, 2020 11:30

Merge branch 'master' into aeschli/themableDebugIcons

a327c55

implement review comments

384d7b5

Merge branch 'master' into aeschli/themableDebugIcons

ac21b36

move down common icons

e052c8e

aeschli merged commit 6a41811 into master Nov 24, 2020

aeschli deleted the aeschli/themableDebugIcons branch November 24, 2020 20:32

github-actions bot locked and limited conversation to collaborators Jan 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

themable debug icons #111183

themable debug icons #111183

aeschli commented Nov 23, 2020 •

edited

Loading

isidorn Nov 23, 2020

aeschli Nov 24, 2020 •

edited

Loading

isidorn Nov 24, 2020

isidorn Nov 23, 2020

isidorn Nov 23, 2020

isidorn Nov 23, 2020

aeschli Nov 24, 2020 •

edited

Loading

isidorn Nov 24, 2020

isidorn Nov 23, 2020

aeschli Nov 24, 2020

isidorn Nov 23, 2020

aeschli Nov 24, 2020

isidorn Nov 23, 2020

aeschli Nov 24, 2020

isidorn Nov 23, 2020

aeschli Nov 24, 2020

isidorn Nov 24, 2020

isidorn commented Nov 23, 2020 •

edited

Loading

themable debug icons #111183

themable debug icons #111183

Conversation

aeschli commented Nov 23, 2020 • edited Loading

Choose a reason for hiding this comment

aeschli Nov 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeschli Nov 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isidorn commented Nov 23, 2020 • edited Loading

aeschli commented Nov 23, 2020 •

edited

Loading

aeschli Nov 24, 2020 •

edited

Loading

aeschli Nov 24, 2020 •

edited

Loading

isidorn commented Nov 23, 2020 •

edited

Loading