[git] Group the context menu items per Git command #8400

[dhuebner]

What it does

Groups menu items in git toolbar menu into sub-menus

How to test

Open Source Control view, open toolbar menu. Grouped menu should be shown:

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

[kittaakos]

Super. Thank you for the patch. Could you please add groups for Pull/Push and keep the order from VS Code?

VS Code:

Theia:

[dhuebner]

@kittaakos
I'm not sure how to define groups in submenus, I need to try it out

[vince-fugnitto]

I have noticed the following logs with the changes:

[vince-fugnitto]

The following comment may be misleading, the main menu is the top-level menu.
We can likely omit the comment entirely.

[dhuebner]

Yes, you are right. I will remove it

[dhuebner]

I have noticed the following logs with the changes:

@vince-fugnitto
I didn't noticed that :(

This is probably because we have PULL and PUSH in favorites group and as submenu entry. Would it be fine to just create the entries with different command id's or is there a way to reference the same command in different menu items?

[vince-fugnitto]

@dhuebner

The error happens when we attempt to register the same command id multiple times:

theia/packages/core/src/common/command.ts

Lines 189 to 198 in b722e4d

	/**
	* Register the given command and handler if present.
	*
	* Throw if a command is already registered for the given command identifier.
	*/
	registerCommand(command: Command, handler?: CommandHandler): Disposable {
	if (this._commands[command.id]) {
	console.warn(`A command ${command.id} is already registered.`);
	return Disposable.NULL;
	}

This happens when we call registerItem:

theia/packages/git/src/browser/git-contribution.ts

Lines 541 to 545 in b722e4d

	const registerItem = (item: Mutable<TabBarToolbarItem>) => {
	const commandId = item.command;
	const id = '__git.tabbar.toolbar.' + commandId;
	const command = this.commands.getCommand(commandId);
	this.commands.registerCommand({ id, iconClass: command && command.iconClass }, {

One solution might need to register a new command which wraps the existing, and is used in the submenu, but there might be others:

export const PULL_DEFAULT_SUBMENU = {
  id: 'git.pull.default.submenu',
};

registry.registerCommand(GIT_COMMANDS.PULL_DEFAULT_SUBMENU, {
  execute: () => registry.executeCommand(GIT_COMMANDS.PULL_DEFAULT.id),
  isEnabled: () => !!this.repositoryTracker.selectedRepository
});

[dhuebner]

@kittaakos
Groups are added to the submenu:

Unfortunately priorities are lost due to missing order property during renderMoreContextMenu in tab-bar-toolbar.tsx, I added TODO comments.

@vince-fugnitto
I also registered the favorites menu entries (Pull/Push) with new ids like you suggested.

[kittaakos]

Theia:

VS Code:

Besides some ordering differences, the Changes menu groups are in sync 👍. Why did you re-declare the Stage All Changes in the Pull, Push submenu? Is it a bug or feature?

Theia:

VS Code:

[dhuebner]

@kittaakos
Thanks for your feedback. I fixed the Fetch command label (Stage All Changes -> Fetch...) and also moved Merge... from favorites to command group.

Regarding the order, I see any chance to do it properly because of alphabetical sorting due to missing SubMenuOptions here: https://github.com/eclipse-theia/theia/pull/9324/files#diff-07529f987e23173e43dafbf57a0b4b625978849167b48205fc30d3f806feec01

Missing order leads to the alphabetical sorting here: https://github.com/eclipse-theia/theia/blob/master/packages/core/src/common/menu.ts#L341

...because the sorting string is the MenuItem label

[kittaakos]

I have verified it on Gitpod, and it worked:

• there were no warnings in the console due to the duplicate command registration,
• all menu items were there, and
• the Git commands I have tried ran successfully.

Thank you, @dhuebner 👍

@vince-fugnitto, could you please check if you're happy with the merge?

[vince-fugnitto]

I confirmed that the menu is correctly populated and organized when testing 👍

[kittaakos]

@dhuebner, can you please squash your commits? Thank you!

[dhuebner]

@kittaakos
Done!