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

Anasshahidd21 · 2020-01-24T16:56:48Z

What it does

With this PR, upon triggering the context menu, the widget is not activated. This helps perform commands on the context menu without actually giving focus to the widget.

Context menu commands analysed and affected with this change

Close
Close Others
Close to the right
Close All
Toggle Maximized
Reveal in Explorer
Collapse

Signed-off-by: Muhammad Anas Shahid muhammad.shahid@ericsson.com

How to test

Use Case	Current Master	Updated Changes
Open two files, right click on the unfocused one and click close	Right clicking the tab, also reveals the tab and makes it the currentWidget	The right clicked tab will `not` be take focus and still will be able to `close the tab`
Open two files, right click on the unfocused one and click close others	Right clicking the tab, also reveals the tab and makes it the currentWidget and then closes the other tabs	The right clicked tab will `not` take focus and still will be able to `close all other tabs`
Open two files, right click on the unfocused one and click close to the right (make sure there are tabs open to the right, else the option wont be enabled)	Right clicking the tab, also reveals the tab and makes it the currentWidget and then closes the other tabs	The right clicked tab will `not` take focus and still will be able to `close the tab available on the right`
Open two files, right click on the unfocused one and `Toggle Maximized`	Right clicking the tab, also reveals the tab and makes it the currentWidget and then `Maximizes the currentWidget`	The right clicked tab will `not` take focus and still will be able to `maximize the right clicked widget`
Open two files, right click on the unfocused one and `Reveal In Explorer`	Right clicking the tab, also reveals the tab and makes it the currentWidget and then `reveals in the file-explorer`	The right clicked tab will `not` take focus and still will be able to `reveal the right clicked tab on the file-explorer`

Note: 

- The above mentioned test cases must also work for the terminal view and the sidebar view

- The keybindings for `Toggle Maximized`, `Collapse` and `Reveal In Explorer` must also work

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

vince-fugnitto

Unfortunately, it does not work in all cases. Sometimes when using the context-menu to close a view, the previously active view is closed instead. There looks to me more work involved:

Here I moved my changelog.md to the sidepanel, and my explorer was previously active.
Using the context-menu to try and close the changelog.md results in the explorer being closed instead.

vince-fugnitto · 2020-01-24T17:26:28Z

The main issue is that it is broken for editors, even in the main-area:

Anasshahidd21 · 2020-01-24T20:31:33Z

@vince-fugnitto I have fixed the issue and am ready for review.

vince-fugnitto

There are still regressions with the following approach versus master.
Commands such as Reveal in Explorer and Toggle Maximized no longer work reliably.

In the following case, the console should be maximized but it is the active editor instead which has an odd UX:

packages/core/src/browser/shell/tab-bars.ts

lmcbout · 2020-01-28T12:41:51Z

There are conflicts. Need to rebase to latest master

Anasshahidd21 · 2020-03-05T17:02:51Z

@vince-fugnitto, can you review the updated changes?

packages/core/src/browser/shell/application-shell.ts

Anasshahidd21 · 2020-03-11T17:13:56Z

@lmcbout can you review?

akosyakov · 2020-03-12T08:11:57Z

@Anasshahidd21 a change breaking context menu is still not reverted, with this PR the current widget won't be proper.

akosyakov · 2020-03-12T08:18:38Z

Reveal in explorer is missing.

If you want to remove an activation of tabs then the proper process:

identify all menu paths for tabs' context menus
identify all added commands
change a command that they can work with event and without
- so test 3 cases for each command in each area:
  - tab in focus
  - tab not in focus
  - command palette
  - maybe more cases if a command used somewhere else like in the toolbar contribution (work with code, analyze it)
write in CHANGELOG why do we break, how it should be fixed by extenders based on your experience above

lmcbout · 2020-03-12T12:11:15Z

As mentioned by @akosyakov , the menu item "Reveal in Explorer " is not available if I put the focus on the preference editor and try to open the context menu after. See video.

packages/navigator/src/browser/navigator-contribution.ts

akosyakov · 2020-03-24T08:29:59Z

packages/core/src/browser/context-menu-service.ts

+import { ApplicationShell } from './shell/application-shell';
+
+@injectable()
+export class ContextMenuService {


It looks weird. ContextMenuService will be responsible to open and close context menus, not a bunch of utility methods.

That should belong to ApplicationShell.

I think we should just delete this class and rewrite commands with existing APIs, there are enough already of them.

vince-fugnitto

@Anasshahidd21 keybindings for tabbar context-menu items are broken with this pull-request.
For example, try to toggle maximized using the keybinding on master versus this pull-request.

CHANGELOG.md

packages/core/src/browser/shell/application-shell.ts

packages/core/src/browser/common-frontend-contribution.ts

Anasshahidd21 · 2020-07-06T14:24:32Z

@akosyakov @vince-fugnitto @lmcbout @marechal-p thank you for your past reviews. I have updated the code to address the comments, and I am ready for another review.

akosyakov · 2020-07-07T07:12:05Z

Changes look good if there are no any other affected commands.

kaiyue0329

The changes work well 👍 ! Tested all the use cases mentioned above for the main view container, the bottom panel and the sidebar view. The keybindings for Toggle Maximized, Collapse and Reveal In Explorer also work correctly.

packages/core/src/browser/common-frontend-contribution.ts

packages/navigator/src/browser/navigator-contribution.ts

vince-fugnitto

I verified the functionality (both browser and electron) and it works well.
I'd like for others to review as well, perhaps you'd like to @kittaakos I believe you opened the original issue.

kittaakos · 2020-08-21T07:07:26Z

Let's fix the linter error:

diff --git a/packages/core/src/browser/common-frontend-contribution.ts b/packages/core/src/browser/common-frontend-contribution.ts
index 33eacca0c..0bd3ca0c5 100644
--- a/packages/core/src/browser/common-frontend-contribution.ts
+++ b/packages/core/src/browser/common-frontend-contribution.ts
@@ -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, MenuContribution, MenuModelRegistry } from '../common/menu';
 import { KeybindingContribution, KeybindingRegistry } from './keybinding';
 import { FrontendApplicationContribution } from './frontend-application';

kittaakos

I tried it out with the electron example, the widgets do not expand on context menu render. I left a few comments, it's up to you if you want to consider them. Thank you for the fix, @Anasshahidd21! Please resolve the linter error.

kittaakos · 2020-08-21T06:48:06Z

packages/core/src/browser/common-frontend-contribution.ts

+     * @param event used to find the selected widget.
+     */
+    private toggleMaximized(event?: Event): void {
+        if (event && event.target) {


[minor]: if (event?.target instanceof HTMLElement), and you can get rid of the static cast below.

kittaakos · 2020-08-21T06:49:45Z

packages/core/src/browser/shell/application-shell.ts

+     * @returns the selected tab-bar, else returns the currentTabBar.
+     */
+    findTabBar(event?: Event): TabBar<Widget> | undefined {
+        if (event && event.target) {


[minor]: Same as above: if (event?.target instanceof HTMLElement).

kittaakos · 2020-08-21T07:01:36Z

packages/core/src/browser/shell/application-shell.ts

+    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')) {


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

vince-fugnitto · 2020-08-25T15:54:41Z

@akosyakov @kittaakos is there any objection to merge the pull-request, or should we wait after the release?

kittaakos · 2020-08-25T15:56:02Z

@kittaakos is there any objection to merge the pull-request

I do not mind merging it. If you're unsure, you can wait.

vince-fugnitto · 2020-08-25T21:10:13Z

I'll merge tomorrow if there are no objections 👍

Fixes: eclipse-theia#4367 Before, right clicking on different menus would focus the menu item and open it, however this should not be the case. There was a dangling code in the handleContextMenu which causes this effect, as it was checking for the id when right clicked and looked up the ID for the menu-item and set the current title to the menu-item. Signed-off-by: Muhammad Anas Shahid <muhammad.shahid@mail.mcgill.ca>

vince-fugnitto · 2020-09-03T16:14:20Z

Merging 🍾

vince-fugnitto reviewed Jan 24, 2020

View reviewed changes

vince-fugnitto requested review from lmcbout and akosyakov January 24, 2020 17:15

Anasshahidd21 force-pushed the anas/issueUI branch from 618b5a8 to 87895ce Compare January 24, 2020 20:29

vince-fugnitto reviewed Jan 24, 2020

View reviewed changes

akosyakov added the shell issues related to the core shell label Jan 25, 2020

akosyakov reviewed Jan 25, 2020

View reviewed changes

packages/core/src/browser/shell/tab-bars.ts Show resolved Hide resolved

Anasshahidd21 force-pushed the anas/issueUI branch from 87895ce to 4cd588a Compare March 5, 2020 17:01

vince-fugnitto reviewed Mar 5, 2020

View reviewed changes

packages/core/src/browser/shell/application-shell.ts Outdated Show resolved Hide resolved

packages/core/src/browser/shell/application-shell.ts Outdated Show resolved Hide resolved

Anasshahidd21 force-pushed the anas/issueUI branch 5 times, most recently from d4b49e1 to faf5af7 Compare March 10, 2020 15:34

Anasshahidd21 force-pushed the anas/issueUI branch 2 times, most recently from fedeb34 to c0137d8 Compare March 23, 2020 15:20

paul-marechal reviewed Mar 23, 2020

View reviewed changes

packages/navigator/src/browser/navigator-contribution.ts Outdated Show resolved Hide resolved

packages/navigator/src/browser/navigator-contribution.ts Outdated Show resolved Hide resolved

Anasshahidd21 force-pushed the anas/issueUI branch 2 times, most recently from d478265 to 6526086 Compare March 23, 2020 19:48

paul-marechal reviewed Mar 23, 2020

View reviewed changes

packages/navigator/src/browser/navigator-contribution.ts Outdated Show resolved Hide resolved

Anasshahidd21 force-pushed the anas/issueUI branch from 6526086 to 32e1a0b Compare March 23, 2020 20:32

akosyakov reviewed Mar 24, 2020

View reviewed changes

vince-fugnitto reviewed Jul 3, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

packages/core/src/browser/shell/application-shell.ts Outdated Show resolved Hide resolved

packages/core/src/browser/common-frontend-contribution.ts Show resolved Hide resolved

Anasshahidd21 force-pushed the anas/issueUI branch 2 times, most recently from b5b9cb6 to 9d33d71 Compare July 6, 2020 05:58

kaiyue0329 approved these changes Jul 8, 2020

View reviewed changes

vince-fugnitto reviewed Aug 6, 2020

View reviewed changes

packages/core/src/browser/common-frontend-contribution.ts Outdated Show resolved Hide resolved

packages/navigator/src/browser/navigator-contribution.ts Outdated Show resolved Hide resolved

Anasshahidd21 force-pushed the anas/issueUI branch from 9d33d71 to 7c31a82 Compare August 6, 2020 15:39

Anasshahidd21 force-pushed the anas/issueUI branch from 7c31a82 to 8a1e0ab Compare August 20, 2020 16:15

vince-fugnitto approved these changes Aug 20, 2020

View reviewed changes

vince-fugnitto requested a review from paul-marechal August 20, 2020 17:22

Anasshahidd21 force-pushed the anas/issueUI branch 3 times, most recently from 9ceaaa3 to f10da77 Compare August 21, 2020 05:55

kittaakos approved these changes Aug 21, 2020

View reviewed changes

Anasshahidd21 force-pushed the anas/issueUI branch 4 times, most recently from 2155a89 to 976a752 Compare August 21, 2020 15:44

Anasshahidd21 force-pushed the anas/issueUI branch from 976a752 to f4f313a Compare August 26, 2020 14:36

vince-fugnitto force-pushed the anas/issueUI branch from f4f313a to d3434e9 Compare August 31, 2020 12:36

vince-fugnitto force-pushed the anas/issueUI branch from d3434e9 to 9d45578 Compare September 3, 2020 14:22

vince-fugnitto merged commit 636b181 into eclipse-theia:master Sep 3, 2020

vince-fugnitto deleted the anas/issueUI branch September 3, 2020 16:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Anasshahidd21 commented Jan 24, 2020 •

edited

Loading

vince-fugnitto left a comment

vince-fugnitto commented Jan 24, 2020

Anasshahidd21 commented Jan 24, 2020

vince-fugnitto left a comment

lmcbout commented Jan 28, 2020

Anasshahidd21 commented Mar 5, 2020

Anasshahidd21 commented Mar 11, 2020

akosyakov commented Mar 12, 2020 •

edited

Loading

akosyakov commented Mar 12, 2020

lmcbout commented Mar 12, 2020

akosyakov Mar 24, 2020

akosyakov Mar 24, 2020

vince-fugnitto left a comment

Anasshahidd21 commented Jul 6, 2020

akosyakov commented Jul 7, 2020

kaiyue0329 left a comment

vince-fugnitto left a comment

kittaakos commented Aug 21, 2020

kittaakos left a comment

kittaakos Aug 21, 2020

kittaakos Aug 21, 2020

kittaakos Aug 21, 2020

vince-fugnitto commented Aug 25, 2020

kittaakos commented Aug 25, 2020

vince-fugnitto commented Aug 25, 2020

vince-fugnitto commented Sep 3, 2020

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

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

Conversation

Anasshahidd21 commented Jan 24, 2020 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

vince-fugnitto left a comment

Choose a reason for hiding this comment

vince-fugnitto commented Jan 24, 2020

Anasshahidd21 commented Jan 24, 2020

vince-fugnitto left a comment

Choose a reason for hiding this comment

lmcbout commented Jan 28, 2020

Anasshahidd21 commented Mar 5, 2020

Anasshahidd21 commented Mar 11, 2020

akosyakov commented Mar 12, 2020 • edited Loading

akosyakov commented Mar 12, 2020

lmcbout commented Mar 12, 2020

akosyakov Mar 24, 2020

Choose a reason for hiding this comment

akosyakov Mar 24, 2020

Choose a reason for hiding this comment

vince-fugnitto left a comment

Choose a reason for hiding this comment

Anasshahidd21 commented Jul 6, 2020

akosyakov commented Jul 7, 2020

kaiyue0329 left a comment

Choose a reason for hiding this comment

vince-fugnitto left a comment

Choose a reason for hiding this comment

kittaakos commented Aug 21, 2020

kittaakos left a comment

Choose a reason for hiding this comment

kittaakos Aug 21, 2020

Choose a reason for hiding this comment

kittaakos Aug 21, 2020

Choose a reason for hiding this comment

kittaakos Aug 21, 2020

Choose a reason for hiding this comment

vince-fugnitto commented Aug 25, 2020

kittaakos commented Aug 25, 2020

vince-fugnitto commented Aug 25, 2020

vince-fugnitto commented Sep 3, 2020

Anasshahidd21 commented Jan 24, 2020 •

edited

Loading

akosyakov commented Mar 12, 2020 •

edited

Loading