Switching menubar display position failed with error

[yiliang114]

Reproduction steps:

1. Use vscode.dev to open a project, such as https://insiders.vscode.dev/github/microsoft/vscode
2. Switch the top menubar from TitleBar to the ActivityBar, or do the opposite.
3. After switching several times, it was found that the switch failed. If you open the console, you will find an error.

Version: 1.87.0-insider
Commit: ee69e28
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/121.0.0.0 Safari/537.36
Embedder: vscode.dev

[yiliang114]

ref: #199744, because command with the same name cannot be registered now.

The process of switching menu bars is accompanied by the destruction and initialization of two custom menu bars from A and B respectively, and their destruction execution order cannot be guaranteed. When one of the instances is destroyed and executed, the initialization of the new instance will have this problem, because initialization means registering a command with the same name.

vscode/src/vs/workbench/browser/parts/titlebar/menubarControl.ts

Line 440 in f7f9fb0

id: `workbench.actions.menubar.focus`,

[bpasero]

I think the issue here is that the menu bar control gets disposed in the one place and then created in the other place (title bar, activity bar) but the order is not guaranteed to be dispose first and create later, as such this action gets first registered a second time and then gets disposed:

vscode/src/vs/workbench/browser/parts/titlebar/menubarControl.ts

Lines 437 to 454 in 67f9158

	this._register(registerAction2(class extends Action2 {
	constructor() {
	super({
	id: `workbench.actions.menubar.focus`,
	title: localize2('focusMenu', 'Focus Application Menu'),
	keybinding: {
	primary: KeyMod.Alt | KeyCode.F10,
	weight: KeybindingWeight.WorkbenchContrib,
	when: IsWebContext
	},
	f1: true
	});
	}
	async run(): Promise<void> {
	that.menubar?.toggleFocus();
	}
	}));

We only recently changed to throwing an error in this case which now breaks this flow, but the issue was there before already...

[yiliang114]

id: workbench.actions.menubar.focus,

Yeah, I found it, too. My original idea was to ensure that it was reasonable to reinstall after the uninstall was completed through some event mechanism, such as use this.titleService.onMenubarVisibilityChange, but that would require listening on both sides.

[yiliang114]

We only recently changed to throwing an error in this case which now breaks this flow, but the issue was there before already...

Yes, Previously command with the same name can be overwritten without causing a problem. The exception thrown now causes the handler to not be executed and the switch fails.