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

fix default Anchor param of context menu #10540

Closed
wants to merge 1 commit into from
Closed

fix default Anchor param of context menu #10540

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
7 changes: 7 additions & 0 deletions packages/core/src/browser/context-menu-renderer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,13 @@ export function toAnchor(anchor: HTMLElement | Coordinate): Anchor {
return anchor instanceof HTMLElement ? { x: anchor.offsetLeft, y: anchor.offsetTop } : anchor;
}

export function isAnchor(arg: unknown): boolean {
if (arg && typeof arg === 'object' && 'x' in arg && 'y' in arg && Object.keys(arg).length === 2) {
return true;
}
return false;
}

export function coordinateFromAnchor(anchor: Anchor): Coordinate {
const { x, y } = anchor instanceof MouseEvent ? { x: anchor.clientX, y: anchor.clientY } : anchor;
return { x, y };
Expand Down
6 changes: 3 additions & 3 deletions packages/debug/src/browser/debug-frontend-application-contribution.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
********************************************************************************/

import { AbstractViewContribution, ApplicationShell, KeybindingRegistry, Widget, CompositeTreeNode, LabelProvider, codicon } from '@theia/core/lib/browser';
import { AbstractViewContribution, ApplicationShell, KeybindingRegistry, Widget, CompositeTreeNode, LabelProvider, codicon, isAnchor } from '@theia/core/lib/browser';
import { injectable, inject } from '@theia/core/shared/inversify';
import { ThemeService } from '@theia/core/lib/browser/theming';
import { MenuModelRegistry, CommandRegistry, MAIN_MENU_BAR, Command, Emitter, Mutable } from '@theia/core/lib/common';
Expand Down Expand Up @@ -640,10 +640,10 @@ export class DebugFrontendApplicationContribution extends AbstractViewContributi
registerCommands(registry: CommandRegistry): void {
super.registerCommands(registry);
registry.registerCommand(DebugCommands.START, {
execute: (config?: DebugSessionOptions) => this.start(false, config)
execute: (config?: DebugSessionOptions) => this.start(false, isAnchor(config) ? undefined : config)
});
registry.registerCommand(DebugCommands.START_NO_DEBUG, {
execute: (config?: DebugSessionOptions) => this.start(true, config)
execute: (config?: DebugSessionOptions) => this.start(true, isAnchor(config) ? undefined : config)
Comment on lines +643 to +646
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to change my mind here, but even though the cause of the problem is that an Anchor is being passed in, but what we really want to guarantee is that the config is an instance of DebugSessionOptions - the problem with the CommandHandler system is that we're allowed to say config?: DebugSessionOption, but in fact anything can be passed in - Anchors, or anything else..

So rather than adding a check for whether this is an Anchor, it would be more correct in this context to implement a check that something is a DebugSessionOptions. Something like.

export namespace DebugSessionOptions {
    export function is(candidate: unknown): candidate is DebugSessionOptions {
        const options = candidate as DebugSessionOptions;
        return typeof options === 'object' && !!candidate && 'configuration' in candidate && DebugConfiguration.is(options.candidate);
    }
}

Again, sorry for the change of direction!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say hold off on this pending a general resolution of the problems introduced by window.menuBarVisibility compact. As I say here, the correct, non-breaking solution is probably just to fix the behavior of the menu introduced by that PR, rather than fix the commands one by one or change the existing behavior of real context menus.

});
registry.registerCommand(DebugCommands.STOP, {
execute: () => this.manager.terminateSessions(),
Expand Down