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.

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

Core: Do no expand the widgets on the side-bars for the context menu #6965

Merged
merged 1 commit into from
Sep 3, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 13 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,18 @@
# Change Log

## v1.6.0

- [core] added functionality for handling context menu for `tab-bars` without activating the shell tab-bar [#6965](https://github.com/eclipse-theia/theia/pull/6965)

<a name="breaking_changes_1.6.0">[Breaking Changes:](#breaking_changes_1.6.0)</a>

- [core] Context-menu for `tab-bars` requires an `Event` to be passed onto it to perform actions without activating the shell tab-bar [#6965](https://github.com/eclipse-theia/theia/pull/6965)
- Removed the logic from `handleContextMenuEvent()` that gives focus to the widget upon opening the context menu, instead functionality added to handle it without activating the widget
- This change triggers the context menu for a given shell tab-bar without the need to activate it
- While registering a command, `Event` should be passed down, if not passed, then the commands will not work correctly as they no longer rely on the activation of tab-bar
- [core] Moved `findTitle()` and `findTabBar()` from `common-frontend-contribution.ts` to `application-shell.ts` [#6965](https://github.com/eclipse-theia/theia/pull/6965)


## v1.5.0 - 27/08/2020

- [application-manager] fixed issue regarding reloading electron windows [#8345](https://github.com/eclipse-theia/theia/pull/8345)
Expand Down
105 changes: 66 additions & 39 deletions packages/core/src/browser/common-frontend-contribution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

import debounce = require('lodash.debounce');
import { injectable, inject } from 'inversify';
import { TabBar, Widget, Title } from '@phosphor/widgets';
import { TabBar, Widget } from '@phosphor/widgets';
import { MAIN_MENU_BAR, SETTINGS_MENU, MenuContribution, MenuModelRegistry } from '../common/menu';
import { KeybindingContribution, KeybindingRegistry } from './keybinding';
import { FrontendApplication, FrontendApplicationContribution } from './frontend-application';
Expand Down Expand Up @@ -621,55 +621,59 @@ export class CommonFrontendContribution implements FrontendApplicationContributi
});
commandRegistry.registerCommand(CommonCommands.CLOSE_TAB, {
isEnabled: (event?: Event) => {
const tabBar = this.findTabBar(event);
const tabBar = this.shell.findTabBar(event);
if (!tabBar) {
return false;
}
const currentTitle = this.findTitle(tabBar, event);
const currentTitle = this.shell.findTitle(tabBar, event);
return currentTitle !== undefined && currentTitle.closable;
},
execute: (event?: Event) => {
const tabBar = this.findTabBar(event)!;
const currentTitle = this.findTitle(tabBar, event);
const tabBar = this.shell.findTabBar(event)!;
const currentTitle = this.shell.findTitle(tabBar, event);
this.shell.closeTabs(tabBar, title => title === currentTitle);
}
});
commandRegistry.registerCommand(CommonCommands.CLOSE_OTHER_TABS, {
isEnabled: (event?: Event) => {
const tabBar = this.findTabBar(event);
const tabBar = this.shell.findTabBar(event);
if (!tabBar) {
return false;
}
const currentTitle = this.findTitle(tabBar, event);
const currentTitle = this.shell.findTitle(tabBar, event);
return tabBar.titles.some(title => title !== currentTitle && title.closable);
},
execute: (event?: Event) => {
const tabBar = this.findTabBar(event)!;
const currentTitle = this.findTitle(tabBar, event);
const tabBar = this.shell.findTabBar(event)!;
const currentTitle = this.shell.findTitle(tabBar, event);
this.shell.closeTabs(tabBar, title => title !== currentTitle && title.closable);
}
});
commandRegistry.registerCommand(CommonCommands.CLOSE_RIGHT_TABS, {
isEnabled: (event?: Event) => {
const tabBar = this.findTabBar(event);
return tabBar !== undefined && tabBar.titles.some((title, index) => index > tabBar.currentIndex && title.closable);
const tabBar = this.shell.findTabBar(event);
if (!tabBar) {
return false;
}
const currentIndex = this.findTitleIndex(tabBar, event);
return tabBar.titles.some((title, index) => index > currentIndex && title.closable);
},
isVisible: (event?: Event) => {
const area = this.findTabArea(event);
return area !== undefined && area !== 'left' && area !== 'right';
},
execute: (event?: Event) => {
const tabBar = this.findTabBar(event)!;
const currentIndex = tabBar.currentIndex;
const tabBar = this.shell.findTabBar(event)!;
const currentIndex = this.findTitleIndex(tabBar, event);
this.shell.closeTabs(tabBar, (title, index) => index > currentIndex && title.closable);
}
});
commandRegistry.registerCommand(CommonCommands.CLOSE_ALL_TABS, {
isEnabled: (event?: Event) => {
const tabBar = this.findTabBar(event);
const tabBar = this.shell.findTabBar(event);
return tabBar !== undefined && tabBar.titles.some(title => title.closable);
},
execute: (event?: Event) => this.shell.closeTabs(this.findTabBar(event)!, title => title.closable)
execute: (event?: Event) => this.shell.closeTabs(this.shell.findTabBar(event)!, title => title.closable)
});
commandRegistry.registerCommand(CommonCommands.CLOSE_MAIN_TAB, {
isEnabled: () => {
Expand Down Expand Up @@ -716,9 +720,9 @@ export class CommonFrontendContribution implements FrontendApplicationContributi
}
});
commandRegistry.registerCommand(CommonCommands.TOGGLE_MAXIMIZED, {
isEnabled: () => this.shell.canToggleMaximized(),
isVisible: () => this.shell.canToggleMaximized(),
execute: () => this.shell.toggleMaximized()
isEnabled: (event?: Event) => this.canToggleMaximized(event),
isVisible: (event?: Event) => this.canToggleMaximized(event),
execute: (event?: Event) => this.toggleMaximized(event)
});

commandRegistry.registerCommand(CommonCommands.SAVE, {
Expand All @@ -743,38 +747,61 @@ export class CommonFrontendContribution implements FrontendApplicationContributi
});
}

private findTabBar(event?: Event): TabBar<Widget> | undefined {
if (event && event.target) {
const tabBar = this.shell.findWidgetForElement(event.target as HTMLElement);
if (tabBar instanceof TabBar) {
return tabBar;
}
}
return this.shell.currentTabBar;
}

private findTabArea(event?: Event): ApplicationShell.Area | undefined {
const tabBar = this.findTabBar(event);
const tabBar = this.shell.findTabBar(event);
if (tabBar) {
return this.shell.getAreaFor(tabBar);
}
return this.shell.currentTabArea;
}

private findTitle(tabBar: TabBar<Widget>, event?: Event): Title<Widget> | undefined {
if (event && event.target) {
let tabNode: HTMLElement | null = event.target as HTMLElement;
while (tabNode && !tabNode.classList.contains('p-TabBar-tab')) {
tabNode = tabNode.parentElement;
/**
* Finds the index of the selected title from the tab-bar.
* @param tabBar: used for providing an array of titles.
* @returns the index of the selected title if it is available in the tab-bar, else returns the index of currently-selected title.
*/
private findTitleIndex(tabBar: TabBar<Widget>, event?: Event): number {
if (event) {
const targetTitle = this.shell.findTitle(tabBar, event);
return targetTitle ? tabBar.titles.indexOf(targetTitle) : tabBar.currentIndex;
}
return tabBar.currentIndex;
}

private canToggleMaximized(event?: Event): boolean {
if (event?.target instanceof HTMLElement) {
const widget = this.shell.findWidgetForElement(event.target);
if (widget) {
return this.shell.mainPanel.contains(widget) || this.shell.bottomPanel.contains(widget);
}
if (tabNode && tabNode.title) {
const title = tabBar.titles.find(t => t.label === tabNode!.title);
if (title) {
return title;
}
return this.shell.canToggleMaximized();
}

/**
* Maximize the bottom or the main dockpanel based on the widget.
* @param event used to find the selected widget.
*/
private toggleMaximized(event?: Event): void {
vince-fugnitto marked this conversation as resolved.
Show resolved Hide resolved
if (event?.target instanceof HTMLElement) {
const widget = this.shell.findWidgetForElement(event.target);
if (widget) {
if (this.shell.mainPanel.contains(widget)) {
this.shell.mainPanel.toggleMaximized();
} else if (this.shell.bottomPanel.contains(widget)) {
this.shell.bottomPanel.toggleMaximized();
}
if (widget instanceof TabBar) {
// reveals the widget when maximized.
const title = this.shell.findTitle(widget, event);
if (title) {
this.shell.revealWidget(title.owner.id);
}
}
}
} else {
this.shell.toggleMaximized();
}
return tabBar.currentTitle || undefined;
}

private isElectron(): boolean {
Expand Down
39 changes: 39 additions & 0 deletions packages/core/src/browser/shell/application-shell.ts
Original file line number Diff line number Diff line change
Expand Up @@ -811,6 +811,45 @@ export class ApplicationShell extends Widget {
return result;
}

/**
* Finds the title widget from the tab-bar.
* @param tabBar used for providing an array of titles.
* @returns the selected title widget, else returns the currentTitle or undefined.
*/
findTitle(tabBar: TabBar<Widget>, event?: Event): Title<Widget> | undefined {
if (event?.target instanceof HTMLElement) {
let tabNode: HTMLElement | null = event.target;
while (tabNode && !tabNode.classList.contains('p-TabBar-tab')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

[minor]: Can we use closest? But I am fine with the current logic too.

tabNode = tabNode.parentElement;
}
if (tabNode && tabNode.title) {
let title = tabBar.titles.find(t => t.caption === tabNode!.title);
if (title) {
return title;
}
title = tabBar.titles.find(t => t.label === tabNode!.title);
if (title) {
return title;
}
}
}
return tabBar.currentTitle || undefined;
}

/**
* Finds the tab-bar widget.
* @returns the selected tab-bar, else returns the currentTabBar.
*/
findTabBar(event?: Event): TabBar<Widget> | undefined {
if (event?.target instanceof HTMLElement) {
const tabBar = this.findWidgetForElement(event.target);
if (tabBar instanceof TabBar) {
return tabBar;
}
}
return this.currentTabBar;
}

/**
* The current widget in the application shell. The current widget is the last widget that
* was active and not yet closed. See the remarks to `activeWidget` on what _active_ means.
Expand Down
12 changes: 0 additions & 12 deletions packages/core/src/browser/shell/tab-bars.ts
Original file line number Diff line number Diff line change
Expand Up @@ -435,18 +435,6 @@ export class TabBarRenderer extends TabBar.Renderer {
if (this.contextMenuRenderer && this.contextMenuPath && event.currentTarget instanceof HTMLElement) {
event.stopPropagation();
event.preventDefault();

if (this.tabBar) {
paul-marechal marked this conversation as resolved.
Show resolved Hide resolved
const id = event.currentTarget.id;
// eslint-disable-next-line no-null/no-null
const title = this.tabBar.titles.find(t => this.createTabId(t) === id) || null;
this.tabBar.currentTitle = title;
this.tabBar.activate();
if (title) {
title.owner.activate();
}
}

this.contextMenuRenderer.render(this.contextMenuPath, event);
}
};
Expand Down
31 changes: 27 additions & 4 deletions packages/navigator/src/browser/navigator-contribution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ import {
PreferenceService,
SelectableTreeNode,
SHELL_TABBAR_CONTEXT_MENU,
Widget
Widget,
Title
} from '@theia/core/lib/browser';
import { FileDownloadCommands } from '@theia/filesystem/lib/browser/download/file-download-command-contribution';
import {
Expand Down Expand Up @@ -260,9 +261,18 @@ export class FileNavigatorContribution extends AbstractViewContribution<FileNavi
execute: () => this.openView({ activate: true })
});
registry.registerCommand(FileNavigatorCommands.REVEAL_IN_NAVIGATOR, {
execute: () => this.openView({ activate: true }).then(() => this.selectWidgetFileNode(this.shell.currentWidget)),
isEnabled: () => Navigatable.is(this.shell.currentWidget),
isVisible: () => Navigatable.is(this.shell.currentWidget)
execute: (event?: Event) => {
const widget = this.findTargetedWidget(event);
this.openView({ activate: true }).then(() => this.selectWidgetFileNode(widget || this.shell.currentWidget));
},
isEnabled: (event?: Event) => {
const widget = this.findTargetedWidget(event);
return widget ? Navigatable.is(widget) : Navigatable.is(this.shell.currentWidget);
},
isVisible: (event?: Event) => {
const widget = this.findTargetedWidget(event);
return widget ? Navigatable.is(widget) : Navigatable.is(this.shell.currentWidget);
}
});
registry.registerCommand(FileNavigatorCommands.TOGGLE_HIDDEN_FILES, {
execute: () => {
Expand Down Expand Up @@ -545,6 +555,19 @@ export class FileNavigatorContribution extends AbstractViewContribution<FileNavi
this.tabbarToolbarRegistry.registerItem(item);
};

/**
* Find the selected widget.
* @returns `widget` of the respective `title` if it exists, else returns undefined.
*/
private findTargetedWidget(event?: Event): Widget | undefined {
let title: Title<Widget> | undefined;
Copy link
Member

Choose a reason for hiding this comment

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

Why don't use shell.findWidgetForElement for event target?

Copy link
Contributor Author

@Anasshahidd21 Anasshahidd21 Mar 24, 2020

Choose a reason for hiding this comment

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

@akosyakov using shell.findWidgetForElement returns the entire tab-bar, so in order to still reveal the right file in the explorer we will have to go through the process of finding the title and returning the widget for that title, imo.

if (event) {
const tab = this.shell.findTabBar(event);
title = tab && this.shell.findTitle(tab, event);
}
return title && title.owner;
}

/**
* Reveals and selects node in the file navigator to which given widget is related.
* Does nothing if given widget undefined or doesn't have related resource.
Expand Down