Remove the old menu implementation #54826

sbatten · 2018-07-22T18:13:21Z

This removes the old menubar implementation to allow macOS and the native menubar experience to take advantage of the new menubar registration. Please test the new branch and review.

isidorn · 2018-07-23T08:30:23Z

@sbatten thanks for the PR. Before I test the new branch could you clarify something:

Why is there still a file menus.ts, isn't the idea to get rid of this file and the old registration?
There are some conflicts with master, can you please resolve those
Since now the native menu takes advantage of the new menu bar registration - is it possible to dynamicly enable / disable actions in the native bar (as we can currently do in custom menu)?

I can test this in great detail, but for the review I only did a birds eye perspective review (comments lnline). @bpasero would be better for the more detailed review (if needed next week once he comes back from vacation)

isidorn · 2018-07-23T08:31:13Z

src/vs/code/electron-main/menubar.ts

@@ -70,7 +57,8 @@ export class Menubar {
 		// this.nativeTabMenuItems = [];

 		this.menuUpdater = new RunOnceScheduler(() => this.doUpdateMenu(), 0);
-		this.keybindingsResolver = instantiationService.createInstance(KeybindingsResolver);
+		// this.keybindingsResolver = instantiationService.createInstance(KeybindingsResolver);


Why is this commented line still needed, same for line 104

isidorn · 2018-07-23T08:31:52Z

src/vs/code/electron-main/menubar.ts

@@ -277,6 +252,15 @@ export class Menubar {
 			menubar.append(gotoMenuItem);
 		}

+		// Terminal


It does not feel right that this component is aware of the terminal menu.
Couldn't this be done fully from the terminal.contribution? What is missing for that?

do you specifically mean for terminal, tasks and preferences? or are you referring to every top-level menu?

I guess the nicest would be that every top level menu item is registered outside and that we do not reference them manually here

isidorn · 2018-07-23T08:32:22Z

src/vs/code/electron-main/menubar.ts

@@ -290,8 +274,8 @@ export class Menubar {
 		const taskMenu = new Menu();
 		const taskMenuItem = new MenuItem({ label: this.mnemonicLabel(nls.localize({ key: 'mTask', comment: ['&& denotes a mnemonic'] }, "&&Tasks")), submenu: taskMenu });

-		if (this.shouldDrawMenu('Task')) {
-			this.setMenuById(taskMenu, 'Task');
+		if (this.shouldDrawMenu('Tasks')) {


Same as for Terminal above.
Also for Preferences below.

isidorn · 2018-07-23T08:33:26Z

src/vs/code/electron-main/menubar.ts

+				this.setMenu(submenu, item.submenu.items);
+				menu.append(submenuItem);
+			} else if (isMenubarMenuItemAction(item)) {
+				if (item.id === 'workbench.action.openRecent') {


I would leave a TODO here since it feels a bit hacky how we are inserting recent menu items

Yea, we do this on both sides for the custom and native menubar experience. maybe we should discuss with a joh a way to do this better with the MenuRegistry in the future

Would you like me to create a debt item so we do not forget about this
fyi @jrieken

what's that about? dynamically computed menu entries?

isidorn · 2018-07-23T08:35:35Z

src/vs/code/electron-main/menubar.ts

 	private nativeTabMenuItems: Electron.MenuItem[];

 	private menubarMenus: IMenubarData = {};

+	private keybindings: { [commandId: string]: IKeybinding };


Why not use a Map<string, IKeybinding>

isidorn · 2018-07-23T08:36:38Z

src/vs/code/electron-main/menubar.ts

+		this.setMenu(menu, this.menubarMenus[menuId].items);
+	}
+
+	private insertRecentMenuItems(menu: Electron.Menu) {


Feels like it deos not belong in this file. this hsould be contributed by the filesActions.contributions imho

same as above?

isidorn · 2018-07-23T08:37:17Z

src/vs/platform/menubar/common/menubar.ts

+export interface IMenubarKeybinding {
+	id: string;
+	label: string;
+	isNative: boolean;


Where is isNative read and what is the purpose

It is used in menubar.ts under electron-main. I was accidentally using IKeybinding still in that file which is now fixed so it should be easier to find.

isidorn · 2018-07-23T08:38:54Z

src/vs/workbench/browser/parts/menubar/menubarPart.ts

-		if (isMacintosh && isWindows) {
-			this.menubarService.updateMenubar(this.windowService.getCurrentWindowId(), this.getMenubarMenus());
-		}
+		this.menubarService.updateMenubar(this.windowService.getCurrentWindowId(), this.getMenubarMenus());


I do not really see a point of a private method called just once that has a one line call. Just my 2 cents from the birds eye perspective

The reason is primarily for symmetry. Where this is called we call either custom or native and the custom one is super long. I could make less functions and more if statements to achieve the same level of readability or just add a comment and remove the private function

Whatever you prefer :)

isidorn · 2018-07-23T08:39:39Z

src/vs/workbench/electron-browser/main.contribution.ts

-	},
-	order: 1
-});
+if (!isMacintosh) {


Can we use when clauses instead of the ugly if brackets,

I would like to do that. I tried but I didn't see platform context keys, but I may have missed them. Are you aware?

I checked and there is none at the moment.
But I would add it somewhere in this file
I would call it platform and it would be windows|linux|macintosh

But we do not have to do it for this PR, we can tackle later. Would you like me to create an issue so we do not forget about it?
fyi @bpasero

@isidorn @sbatten this just landed via #54894

@bpasero nice. @sbatten can you look into using this context keys so we do not have the ugly if clauses.

sbatten · 2018-07-23T15:40:12Z

In response to your initial comment: I will remove the menus.ts and possibly keyboard.ts as they are no longer used. Yes, the native menus will get the same dynamic enablement features as the custom menus with this change. @bpasero had discussed that this change was desired to get some coverage in insiders and recommended you as the reviewer, but good testing on our part before merging is necessary.

isidorn · 2018-07-23T15:45:03Z

@sbatten great, just ping me once all those files are removed and I will test this out on mac.
In my view we should create a test plan item complexity 4 for this on all three platforms. In the endgame week we can decide if we want to ship this or not (in case of too many issues)

isidorn · 2018-07-24T11:00:40Z

Tried it out on mac, here are the issues

Error on startup
Preferences menu is top level (also in Code-Insiders as it should so it is duplicated)

Apart from that seems to work fine. Let me know if you want me to try it out on other platforms.

isidorn · 2018-07-24T11:05:39Z

Thinking about going forward and what we do for this milestone my recommendation is to have some setting for windows, like workbench.customMenu which would be by default false and only temporary there.
So for all platforms we would release the new registration, but the new windows visuals would be under a setting for one milestone. I am leaning towards this protective approach since people are very passionate about menus and this is playing it safe.

This flag does not have to be a part of this PR but something to keep in mind.

Also let me know if I can help in any way.
fyi @dbaeumer @joaomoreno who self hosted

sbatten · 2018-07-25T07:12:56Z

@isidorn fixed issues, please merge if ok, thanks for the review

isidorn · 2018-07-25T08:39:42Z

Looks great, nice work! I have resolved some conflicts and am merging.

sbatten and others added 3 commits July 19, 2018 16:24

removing the old menubar implementation

dbc8505

some fixes for mac menus

3ce86b4

fix keybinding resolution

d332988

sbatten requested a review from isidorn July 22, 2018 18:13

isidorn added this to the July 2018 milestone Jul 23, 2018

isidorn assigned isidorn and sbatten Jul 23, 2018

isidorn requested changes Jul 23, 2018

View reviewed changes

isidorn removed this from the July 2018 milestone Jul 23, 2018

sbatten added 4 commits July 23, 2018 10:55

remove unused code

108190f

Merge branch 'master' into sbatten/removeOldMenu

0acac70

add go submenus

5f87eae

debug submenu

c94682d

fixing bugs

a446812

merge master

f8c92e4

isidorn merged commit d63937a into master Jul 25, 2018

isidorn deleted the sbatten/removeOldMenu branch July 25, 2018 08:39

isidorn added this to the July 2018 milestone Jul 25, 2018

isidorn added the workbench-menu label Jul 25, 2018

sbatten mentioned this pull request Jul 26, 2018

Menu: missing sub menus #54236

Closed

isidorn mentioned this pull request Jul 26, 2018

Remove references to specific menu entries from menubar.ts #55149

Closed

github-actions bot locked and limited conversation to collaborators Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove the old menu implementation #54826

Remove the old menu implementation #54826

sbatten commented Jul 22, 2018

isidorn commented Jul 23, 2018

isidorn Jul 23, 2018

isidorn Jul 23, 2018

sbatten Jul 23, 2018

isidorn Jul 24, 2018

isidorn Jul 23, 2018

isidorn Jul 23, 2018

sbatten Jul 23, 2018

isidorn Jul 24, 2018

jrieken Jul 24, 2018

isidorn Jul 23, 2018

isidorn Jul 23, 2018

sbatten Jul 23, 2018

isidorn Jul 23, 2018

sbatten Jul 23, 2018

isidorn Jul 23, 2018

sbatten Jul 23, 2018

isidorn Jul 24, 2018

isidorn Jul 23, 2018

sbatten Jul 23, 2018

isidorn Jul 24, 2018

bpasero Aug 7, 2018

isidorn Aug 13, 2018

sbatten Aug 13, 2018

sbatten commented Jul 23, 2018 •

edited

Loading

isidorn commented Jul 23, 2018

isidorn commented Jul 24, 2018

isidorn commented Jul 24, 2018 •

edited

Loading

sbatten commented Jul 25, 2018

isidorn commented Jul 25, 2018

Remove the old menu implementation #54826

Remove the old menu implementation #54826

Conversation

sbatten commented Jul 22, 2018

isidorn commented Jul 23, 2018

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

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

Choose a reason for hiding this comment

sbatten commented Jul 23, 2018 • edited Loading

isidorn commented Jul 23, 2018

isidorn commented Jul 24, 2018

isidorn commented Jul 24, 2018 • edited Loading

sbatten commented Jul 25, 2018

isidorn commented Jul 25, 2018

sbatten commented Jul 23, 2018 •

edited

Loading

isidorn commented Jul 24, 2018 •

edited

Loading