Skip to content

Commit

Permalink
Contextmenu duplicates workbenchactionexecuted events (fix #172640) (
Browse files Browse the repository at this point in the history
  • Loading branch information
bpasero authored Jun 6, 2023
1 parent ad34af0 commit 3da8c69
Show file tree
Hide file tree
Showing 9 changed files with 33 additions and 41 deletions.
26 changes: 10 additions & 16 deletions src/vs/base/browser/ui/dropdown/dropdown.ts
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
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
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
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
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
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
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
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
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

0 comments on commit 3da8c69

Please sign in to comment.