Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Contextmenu duplicates workbenchactionexecuted events (fix #172640) #184299

Merged
merged 1 commit into from
Jun 6, 2023
Merged

Contextmenu duplicates workbenchactionexecuted events (fix #172640) #184299

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 10 additions & 16 deletions src/vs/base/browser/ui/dropdown/dropdown.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,24 +158,17 @@ export interface IDropdownMenuOptions extends IBaseDropdownOptions {
readonly actionProvider?: IActionProvider;
menuClassName?: string;
menuAsChild?: boolean; // scope down for #99448
readonly skipTelemetry?: boolean;
}

export class DropdownMenu extends BaseDropdown {
private _contextMenuProvider: IContextMenuProvider;
private _menuOptions: IMenuOptions | undefined;
private _actions: readonly IAction[] = [];
private actionProvider?: IActionProvider;
private menuClassName: string;
private menuAsChild?: boolean;

constructor(container: HTMLElement, options: IDropdownMenuOptions) {
super(container, options);
constructor(container: HTMLElement, private readonly _options: IDropdownMenuOptions) {
super(container, _options);

this._contextMenuProvider = options.contextMenuProvider;
this.actions = options.actions || [];
this.actionProvider = options.actionProvider;
this.menuClassName = options.menuClassName || '';
this.menuAsChild = !!options.menuAsChild;
this.actions = _options.actions || [];
}

set menuOptions(options: IMenuOptions | undefined) {
Expand All @@ -187,8 +180,8 @@ export class DropdownMenu extends BaseDropdown {
}

private get actions(): readonly IAction[] {
if (this.actionProvider) {
return this.actionProvider.getActions();
if (this._options.actionProvider) {
return this._options.actionProvider.getActions();
}

return this._actions;
Expand All @@ -203,17 +196,18 @@ export class DropdownMenu extends BaseDropdown {

this.element.classList.add('active');

this._contextMenuProvider.showContextMenu({
this._options.contextMenuProvider.showContextMenu({
getAnchor: () => this.element,
getActions: () => this.actions,
getActionsContext: () => this.menuOptions ? this.menuOptions.context : null,
getActionViewItem: (action, options) => this.menuOptions && this.menuOptions.actionViewItemProvider ? this.menuOptions.actionViewItemProvider(action, options) : undefined,
getKeyBinding: action => this.menuOptions && this.menuOptions.getKeyBinding ? this.menuOptions.getKeyBinding(action) : undefined,
getMenuClassName: () => this.menuClassName,
getMenuClassName: () => this._options.menuClassName || '',
onHide: () => this.onHide(),
actionRunner: this.menuOptions ? this.menuOptions.actionRunner : undefined,
anchorAlignment: this.menuOptions ? this.menuOptions.anchorAlignment : AnchorAlignment.LEFT,
domForShadowRoot: this.menuAsChild ? this.element : undefined
domForShadowRoot: this._options.menuAsChild ? this.element : undefined,
skipTelemetry: this._options.skipTelemetry
});
}

Expand Down
4 changes: 3 additions & 1 deletion src/vs/base/browser/ui/dropdown/dropdownActionViewItem.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ export interface IDropdownMenuActionViewItemOptions extends IBaseActionViewItemO
readonly classNames?: string[] | string;
readonly anchorAlignmentProvider?: IAnchorAlignmentProvider;
readonly menuAsChild?: boolean;
readonly skipTelemetry?: boolean;
}

export class DropdownMenuActionViewItem extends BaseActionViewItem {
Expand Down Expand Up @@ -101,7 +102,8 @@ export class DropdownMenuActionViewItem extends BaseActionViewItem {
labelRenderer: labelRenderer,
menuAsChild: this.options.menuAsChild,
actions: isActionsArray ? this.menuActionsOrProvider as IAction[] : undefined,
actionProvider: isActionsArray ? undefined : this.menuActionsOrProvider as IActionProvider
actionProvider: isActionsArray ? undefined : this.menuActionsOrProvider as IActionProvider,
skipTelemetry: this.options.skipTelemetry
};

this.dropdownMenu = this._register(new DropdownMenu(container, options));
Expand Down
7 changes: 5 additions & 2 deletions src/vs/base/browser/ui/toolbar/toolbar.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ export interface IToolBarOptions {
renderDropdownAsChildElement?: boolean;
moreIcon?: ThemeIcon;
allowContextMenu?: boolean;
skipTelemetry?: boolean;
}

/**
Expand Down Expand Up @@ -78,7 +79,8 @@ export class ToolBar extends Disposable {
keybindingProvider: this.options.getKeyBinding,
classNames: ThemeIcon.asClassNameArray(options.moreIcon ?? Codicon.toolBarMore),
anchorAlignmentProvider: this.options.anchorAlignmentProvider,
menuAsChild: !!this.options.renderDropdownAsChildElement
menuAsChild: !!this.options.renderDropdownAsChildElement,
skipTelemetry: this.options.skipTelemetry
}
);
this.toggleMenuActionViewItem.setActionContext(this.actionBar.context);
Expand Down Expand Up @@ -106,7 +108,8 @@ export class ToolBar extends Disposable {
keybindingProvider: this.options.getKeyBinding,
classNames: action.class,
anchorAlignmentProvider: this.options.anchorAlignmentProvider,
menuAsChild: !!this.options.renderDropdownAsChildElement
menuAsChild: !!this.options.renderDropdownAsChildElement,
skipTelemetry: this.options.skipTelemetry
}
);
result.setActionContext(this.actionBar.context);
Expand Down
7 changes: 5 additions & 2 deletions src/vs/platform/actions/browser/toolbar.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,13 +95,15 @@ export class WorkbenchToolBar extends ToolBar {
..._options,
// mandatory (overide options)
allowContextMenu: true,
skipTelemetry: typeof _options?.telemetrySource === 'string',
});

// telemetry logic
if (_options?.telemetrySource) {
const telemetrySource = _options?.telemetrySource;
if (telemetrySource) {
this._store.add(this.actionBar.onDidRun(e => telemetryService.publicLog2<WorkbenchActionExecutedEvent, WorkbenchActionExecutedClassification>(
'workbenchActionExecuted',
{ id: e.action.id, from: _options!.telemetrySource! })
{ id: e.action.id, from: telemetrySource })
));
}
}
Expand Down Expand Up @@ -235,6 +237,7 @@ export class WorkbenchToolBar extends ToolBar {
// add context menu actions (iff appicable)
menuId: this._options?.contextMenu,
menuActionOptions: { renderShortTitle: true, ...this._options?.menuOptions },
skipTelemetry: typeof this._options?.telemetrySource === 'string',
contextKeyService: this._contextKeyService,
});
}));
Expand Down
3 changes: 0 additions & 3 deletions src/vs/workbench/browser/parts/auxiliarybar/auxiliaryBarPart.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import { IInstantiationService } from 'vs/platform/instantiation/common/instanti
import { IKeybindingService } from 'vs/platform/keybinding/common/keybinding';
import { INotificationService } from 'vs/platform/notification/common/notification';
import { IStorageService } from 'vs/platform/storage/common/storage';
import { ITelemetryService } from 'vs/platform/telemetry/common/telemetry';
import { contrastBorder } from 'vs/platform/theme/common/colorRegistry';
import { IThemeService } from 'vs/platform/theme/common/themeService';
import { Extensions as PaneCompositeExtensions } from 'vs/workbench/browser/panecomposite';
Expand Down Expand Up @@ -46,7 +45,6 @@ export class AuxiliaryBarPart extends BasePanelPart {
constructor(
@INotificationService notificationService: INotificationService,
@IStorageService storageService: IStorageService,
@ITelemetryService telemetryService: ITelemetryService,
@IContextMenuService contextMenuService: IContextMenuService,
@IWorkbenchLayoutService layoutService: IWorkbenchLayoutService,
@IKeybindingService keybindingService: IKeybindingService,
Expand All @@ -60,7 +58,6 @@ export class AuxiliaryBarPart extends BasePanelPart {
super(
notificationService,
storageService,
telemetryService,
contextMenuService,
layoutService,
keybindingService,
Expand Down
14 changes: 5 additions & 9 deletions src/vs/workbench/browser/parts/compositePart.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { Emitter } from 'vs/base/common/event';
import { isCancellationError } from 'vs/base/common/errors';
import { ActionsOrientation, IActionViewItem, prepareActions } from 'vs/base/browser/ui/actionbar/actionbar';
import { ProgressBar } from 'vs/base/browser/ui/progressbar/progressbar';
import { IAction, WorkbenchActionExecutedEvent, WorkbenchActionExecutedClassification } from 'vs/base/common/actions';
import { IAction } from 'vs/base/common/actions';
import { Part, IPartOptions } from 'vs/workbench/browser/part';
import { Composite, CompositeRegistry } from 'vs/workbench/browser/composite';
import { IComposite } from 'vs/workbench/common/composite';
Expand All @@ -21,7 +21,6 @@ import { IContextMenuService } from 'vs/platform/contextview/browser/contextView
import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation';
import { ServiceCollection } from 'vs/platform/instantiation/common/serviceCollection';
import { IProgressIndicator, IEditorProgressService } from 'vs/platform/progress/common/progress';
import { ITelemetryService } from 'vs/platform/telemetry/common/telemetry';
import { IKeybindingService } from 'vs/platform/keybinding/common/keybinding';
import { IThemeService } from 'vs/platform/theme/common/themeService';
import { INotificationService } from 'vs/platform/notification/common/notification';
Expand Down Expand Up @@ -69,14 +68,13 @@ export abstract class CompositePart<T extends Composite> extends Part {
private titleLabel: ICompositeTitleLabel | undefined;
private progressBar: ProgressBar | undefined;
private contentAreaSize: Dimension | undefined;
private readonly telemetryActionsListener = this._register(new MutableDisposable());
private readonly actionsListener = this._register(new MutableDisposable());
private currentCompositeOpenToken: string | undefined;
private boundarySashes: IBoundarySashes | undefined;

constructor(
private readonly notificationService: INotificationService,
protected readonly storageService: IStorageService,
private readonly telemetryService: ITelemetryService,
protected readonly contextMenuService: IContextMenuService,
layoutService: IWorkbenchLayoutService,
protected readonly keybindingService: IKeybindingService,
Expand Down Expand Up @@ -263,15 +261,12 @@ export abstract class CompositePart<T extends Composite> extends Part {
actionsBinding();

// Action Run Handling
this.telemetryActionsListener.value = toolBar.actionRunner.onDidRun(e => {
this.actionsListener.value = toolBar.actionRunner.onDidRun(e => {

// Check for Error
if (e.error && !isCancellationError(e.error)) {
this.notificationService.error(e.error);
}

// Log in telemetry
this.telemetryService.publicLog2<WorkbenchActionExecutedEvent, WorkbenchActionExecutedClassification>('workbenchActionExecuted', { id: e.action.id, from: this.nameForTelemetry });
});

// Indicate to composite that it is now visible
Expand Down Expand Up @@ -405,7 +400,8 @@ export abstract class CompositePart<T extends Composite> extends Part {
orientation: ActionsOrientation.HORIZONTAL,
getKeyBinding: action => this.keybindingService.lookupKeybinding(action.id),
anchorAlignmentProvider: () => this.getTitleAreaDropDownAnchorAlignment(),
toggleMenuTitle: localize('viewsAndMoreActions', "Views and More Actions...")
toggleMenuTitle: localize('viewsAndMoreActions', "Views and More Actions..."),
telemetrySource: this.nameForTelemetry
}));

this.collectCompositeActions()();
Expand Down
3 changes: 0 additions & 3 deletions src/vs/workbench/browser/parts/panel/panelPart.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,6 @@ export abstract class BasePanelPart extends CompositePart<PaneComposite> impleme
constructor(
@INotificationService notificationService: INotificationService,
@IStorageService storageService: IStorageService,
@ITelemetryService telemetryService: ITelemetryService,
@IContextMenuService contextMenuService: IContextMenuService,
@IWorkbenchLayoutService layoutService: IWorkbenchLayoutService,
@IKeybindingService keybindingService: IKeybindingService,
Expand All @@ -155,7 +154,6 @@ export abstract class BasePanelPart extends CompositePart<PaneComposite> impleme
super(
notificationService,
storageService,
telemetryService,
contextMenuService,
layoutService,
keybindingService,
Expand Down Expand Up @@ -932,7 +930,6 @@ export class PanelPart extends BasePanelPart {
super(
notificationService,
storageService,
telemetryService,
contextMenuService,
layoutService,
keybindingService,
Expand Down
6 changes: 2 additions & 4 deletions src/vs/workbench/browser/parts/sidebar/sidebarPart.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import { IWorkbenchLayoutService, Parts, Position as SideBarPosition } from 'vs/
import { SidebarFocusContext, ActiveViewletContext } from 'vs/workbench/common/contextkeys';
import { IStorageService } from 'vs/platform/storage/common/storage';
import { IContextMenuService } from 'vs/platform/contextview/browser/contextView';
import { ITelemetryService } from 'vs/platform/telemetry/common/telemetry';
import { IKeybindingService } from 'vs/platform/keybinding/common/keybinding';
import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation';
import { Event, Emitter } from 'vs/base/common/event';
Expand Down Expand Up @@ -85,7 +84,6 @@ export class SidebarPart extends CompositePart<PaneComposite> implements IPaneCo
constructor(
@INotificationService notificationService: INotificationService,
@IStorageService storageService: IStorageService,
@ITelemetryService telemetryService: ITelemetryService,
@IContextMenuService contextMenuService: IContextMenuService,
@IWorkbenchLayoutService layoutService: IWorkbenchLayoutService,
@IKeybindingService keybindingService: IKeybindingService,
Expand All @@ -98,7 +96,6 @@ export class SidebarPart extends CompositePart<PaneComposite> implements IPaneCo
super(
notificationService,
storageService,
telemetryService,
contextMenuService,
layoutService,
keybindingService,
Expand Down Expand Up @@ -290,7 +287,8 @@ export class SidebarPart extends CompositePart<PaneComposite> implements IPaneCo
getAnchor: () => anchor,
getActions: () => contextMenuActions.slice(),
getActionViewItem: action => this.actionViewItemProvider(action),
actionRunner: activeViewlet.getActionRunner()
actionRunner: activeViewlet.getActionRunner(),
skipTelemetry: true
});
}
}
Expand Down
4 changes: 3 additions & 1 deletion src/vs/workbench/services/contextmenu/electron-sandbox/contextmenuService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,9 @@ class NativeContextMenuService extends Disposable implements IContextMenuService
}

private async runAction(actionRunner: IActionRunner, actionToRun: IAction, delegate: IContextMenuDelegate, event: IContextMenuEvent): Promise<void> {
this.telemetryService.publicLog2<WorkbenchActionExecutedEvent, WorkbenchActionExecutedClassification>('workbenchActionExecuted', { id: actionToRun.id, from: 'contextMenu' });
if (!delegate.skipTelemetry) {
this.telemetryService.publicLog2<WorkbenchActionExecutedEvent, WorkbenchActionExecutedClassification>('workbenchActionExecuted', { id: actionToRun.id, from: 'contextMenu' });
}

const context = delegate.getActionsContext ? delegate.getActionsContext(event) : undefined;

Expand Down