Bad wording/casing of command title

[jrieken]

Testing #133550

Without reading the TPI I would have not understood what this means: "Toggle size to content width". Do I toggle the size or what? How about "Fixed Content Width" and a check mark that represents on/off. IMO a check mark will help a lot since there is no immediate effect when toggling the mode (e.g existing lines don't get re-rendered)

[meganrogge]

@jrieken we tried using the toggled property and couldn't get it to show up in the context menu. tried modeling the View -> Show Breadcrumbs checkmark to no avail

[jrieken]

@jrieken we tried using the toggled property and couldn't get it to show up in the context menu.

Details? This is working well in many other places

[TarunavBA]

Testing #133550

Without reading the TPI I would have not understood what this means: "Toggle size to content width". Do I toggle the size or what? How about "Fixed Content Width" and a check mark that represents on/off. IMO a check mark will help a lot since there is no immediate effect when toggling the mode (e.g existing lines don't get re-rendered)

@jrieken what theme do u use ? Can you provide the name or json?

[jrieken]

It's Github Light Default at version 4.x (I didn't like the 5.x changes they made). Also these tweaks

	"workbench.colorCustomizations": {
		"sash.hoverBorder": "#5a505042",
		"statusBarItem.remoteBackground": "#00000000",
		"statusBarItem.remoteForeground": "#55810e",
		"list.focusOutline": "#00000000",
		"editorWhitespace.foreground": "#a8929256"
	},

[TarunavBA]

It's Github Light Default at version 4.x (I didn't like the 5.x changes they made). Also these tweaks

	"workbench.colorCustomizations": {
		"sash.hoverBorder": "#5a505042",
		"statusBarItem.remoteBackground": "#00000000",
		"statusBarItem.remoteForeground": "#55810e",
		"list.focusOutline": "#00000000",
		"editorWhitespace.foreground": "#a8929256"
	},

@jrieken Thanks! I'll surely check it out! 🎉

[meganrogge]

@jrieken the checkmark doesn't show up

[jrieken]

I cannot help you, when you show me gifs :-) Do you already use toggled? Do you update the right context key service? Do you know how to debug these things? Do you know F1 > Inspect Context keys?

[meganrogge]

you'll see the code responsible for it in the window in the gif

registerAction2(class extends Action2 {
		constructor() {
			super({
				id: TerminalCommandId.SizeToContentWidthInstance,
				title: terminalStrings.toggleSizeToContentWidth,
				f1: false,
				category,
				toggled: TerminalContextKeys.terminalHasFixedWidth,
				precondition: ContextKeyExpr.and(TerminalContextKeys.processSupported, TerminalContextKeys.focus)
			});
		}
		async run(accessor: ServicesAccessor) {
			return getSelectedInstances(accessor)?.[0].toggleSizeToContentWidth();
		}
	});

[meganrogge]

When I inspect that context key, the value is correctly updating. I don't think this kind of menu supports the checkmark, which I concluded when I originally tried to do this.

[jrieken]

The menu does support it. I suspect that the TerminalContextKeys.processSupported is broken because from time to time I also see Cmd+K not clearing the terminal (and that command uses the same context key)

[meganrogge]

just commented out the precondition and it still doesn't work

[jrieken]

Yeah, sorry worng hint. The precondition is never used for the check mark. It's only the toggled-condition, e.g terminalHasFixedWidth, and the context key inspector tells me its false no matter how often I toggle

[meganrogge]

in the run action when I log the value of that context key, it's correct. not sure why the value is false when you inspect it.

vscode/src/vs/workbench/contrib/terminal/browser/terminalActions.ts

Lines 1881 to 1883 in 246c663

	async run(accessor: ServicesAccessor) {
	return getSelectedInstances(accessor)?.[0].toggleSizeToContentWidth();
	}

I know the context key is updating correctly because otherwise toggling wouldn't happen as you'll see here

vscode/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts

Lines 1923 to 1949 in 75e3411

	async toggleSizeToContentWidth(): Promise<void> {
	if (!this._xterm?.buffer.active) {
	return;
	}
	if (this._terminalHasFixedWidth.get() === true) {
	this._terminalHasFixedWidth.set(false);
	this._fixedCols = undefined;
	this._fixedRows = undefined;
	this._hasScrollBar = false;
	this._initDimensions();
	await this._resize();
	this._horizontalScrollbar?.setScrollDimensions({ scrollWidth: 0 });
	} else {
	let maxCols = 0;
	for (let i = this._xterm.buffer.active.viewportY; i < this._xterm.buffer.active.length; i++) {
	const lineWidth = this._getLineWidth(i, this._xterm.buffer.active);
	maxCols = Math.max(maxCols, lineWidth.width || 0);
	i = lineWidth.newIndex;
	}
	maxCols = Math.min(maxCols, Constants.MaxSupportedCols);
	if (maxCols > this.maxCols) {
	this._fixedCols = maxCols;
	await this._addScrollbar();
	}
	}
	this.focus();
	}

[meganrogge]

Also if I use a different context key - notFindVisible for instance - even when find isn't visible the checkmark isn't appearing.

[meganrogge]

reverting as it's more complicated than just using that context key since that is shared between instances. for now, we'll just keep toggle. I'll work on this next iteration.

[meganrogge]

started work on https://github.com/microsoft/vscode/tree/merogge/checkmark