Open editors widget in navigator #9284

kenneth-marut-work · 2021-03-30T22:13:45Z

What it does

Fixes: #8936
Adds 'Opened Editors' panel into the Navigator's layout, hidden by default

Includes the following functionality:

Shows all opened saveable editors
Provides SCM, Problem, dirty, and preview state decorators
Provides caption affix to indicate file's workspace root, as well as relative path
Adds buttons to close individual editors as well as toolbar buttons to Close All/Save All
Adds context menu options to tree nodes to perform actions including: Open in Terminal, Copy Path, Save, Compare, Close All, Close Others
Tracks active editors and will also activate editors when tree nodes are selected, pins preview widget when doubleclicked
Follows VSCode's grouping convention (i.e. GROUP 1, GROUP 2) to main area editor widgets. If widgets are placed in other shell areas, they are grouped by area (main, bottom, left, right)
Adds tree node items for "Save All" and "Close All" on group nodes and area nodes
Will update ordering of widgets when they are rearranged in a tabbar

Known Issues/Areas for Improvement:

Currently this widget is only tracking NavigatableWidgets (editor widgets) to avoid too much noise, in VSCode it seems that any 'main' area widget is tracked, but this system does not align perfectly with Theia's since we can dock widgets anywhere.
VSCode provides additional toolbar buttons including 'New Untitled File', and 'Toggle Vertical/Horizontal Layout'
VSCode will display a badge decoration in the toolbar when there are any unsaved editors:
VSCode auto-sizes its panel layout based on the widget's contents, I'm not quite sure how to achieve this easily with Phosphor's PanelLayout.
When widgets are rearranged on the left/right toolbars, the ordering will not update (it seems that the left/right DockPanel's layoutModified listener behaves differently from the main/bottom dock panel

How to test

Set Up:

Pull branch and build
Open a single-root workspace and open some files
Right click the file explorer's tabbar and select "Open Editors" to enable the widget

Widget Tracking:

Open some files and observe that editors are tracked and added
Click on the nodes and observe the their corresponding widgets are activated
Close widgets from the tabbar, are they removed from the tree view?
Dock widgets in different panes, split widgets in the main area, observe the grouping strategy. Does this make sense?
Try out some of the grouping toolbar items like "Close Group" and "Save All in Group"

Decorations:

Start editing a file, observe the the file is marked as dirty
If a widget is opened in Preview mode, it should appear in italics (reflecting the Main Area tabbar state) double click on its node to pin the widget, it should be displayed as normal
Attempt to close the file using the 'x' icon, does it behave as expected?
Try these steps in a multiroot workspace, observe that caption suffixes are added that indicate the workspace root as well as its relative path
Introducing invalid syntax in a file should decorate the node using the problem decorator to match its appearance in the file explorer
Introducing changes will decorate the node with the SCM decorator

Actions:

Use the toolbar icons and verify they behave as expected
Use the context menu and verify the items behave as expected

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

colin-grant-work

Looking very nice. I have a few comments on the code

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

packages/navigator/src/browser/open-editors-widget/navigator-open-editors-decorator-service.ts

packages/navigator/src/browser/open-editors-widget/navigator-open-editors-file-decorator.ts

packages/navigator/src/browser/open-editors-widget/navigator-open-editors-tree-model.ts

packages/navigator/src/browser/open-editors-widget/navigator-open-editors-widget.tsx

packages/navigator/src/browser/open-editors-widget/navigator-open-editors-file-decorator.ts

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

vince-fugnitto

I did a high level pass of the pull-request, the main concern was the new coupling introduced by the changes.

I found an issue with the current styling, for different icon-themes (such as the seti) we encounter some problems:

open-editors-ui.mp4

packages/navigator/package.json

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

kenneth-marut-work · 2021-04-23T18:12:14Z

I did a high level pass of the pull-request, the main concern was the new coupling introduced by the changes.

I found an issue with the current styling, for different icon-themes (such as the seti) we encounter some problems:

open-editors-ui.mp4

I've updated the styling to satisfy changing the file icon themes. There is a bit of odd behavior when switching to Seti in particular as the indentation is not consistent with other themes (you can see this on the navigator tree as well), but does not cause any overlap with the decorator. Let me know if this is sufficient. Thanks!

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

colin-grant-work

This seems to be working well for me, and the dependencies on the editor and editor-preview packages have been removed. One small hiccup is the size of widget - it doesn't seem to be adhering to the weight ratio specified - and the handling of multiple instances of the same editor widget.

packages/navigator/src/browser/navigator-widget-factory.ts

kenneth-marut-work · 2021-05-12T20:04:12Z

This seems to be working well for me, and the dependencies on the editor and editor-preview packages have been removed. One small hiccup is the size of widget - it doesn't seem to be adhering to the weight ratio specified - and the handling of multiple instances of the same editor widget.

Thanks for taking another look @colin-grant-work. I've addressed your feedback, and also left a comment in the code regarding the quoted issue above.

kenneth-marut-work · 2021-06-30T18:53:57Z

@colin-grant-work I've pushed an update to this PR that adds grouping functionality to the OpenEditors widget based on my best attempt at reconciling VSCode's OpenEditorsWidget behavior with Theia's ability to dock widgets anywhere. I've also added the corresponding toolbar icons to the grouping nodes that allow for "Close Group" and "Save all in group", and added listeners so that the tree is reordered when tabs are rearranged. Please give this a try and let me know if you have any suggestions

colin-grant-work

Looking pretty good. The behavior is pretty much spot on, just some minor comments about small display items and some stuff in the code.

packages/navigator/src/browser/open-editors-widget/navigator-open-editors-widget.tsx

packages/navigator/src/browser/open-editors-widget/navigator-open-editors-decorator-service.ts

packages/editor-preview/src/browser/editor-preview-tree-decorator.ts

packages/editor-preview/src/browser/editor-preview-manager.ts

packages/editor-preview/src/browser/editor-preview-tree-decorator.ts

packages/navigator/src/browser/open-editors-widget/navigator-open-editors-tree-model.ts

packages/terminal/src/browser/terminal-frontend-contribution.ts

packages/editor-preview/src/browser/editor-preview-tree-decorator.ts

colin-grant-work

This is working well to me, and the code looks reasonable. My greatest desideratum would be to follow VSCode in resizing the tree to only ever take up the amount of space it needs, but that can be tackled in a follow up PR.

kenneth-marut-work · 2021-07-09T14:59:35Z

Hey @vince-fugnitto, I've made some considerable updates to this MR, would you be able to give it a test whenever you have free time?

My greatest desideratum would be to follow VSCode in resizing the tree to only ever take up the amount of space it needs, but that can be tackled in a follow up PR.

Regarding @colin-grant-work's comment above about the automatic resizing of a ViewContainer part, do you (@vince-fugnitto) have any suggestions on how to do that with Phosphor's built in methods? I couldn't seem to find a convenient way to do this.

Thanks!

vince-fugnitto · 2021-07-09T15:01:08Z

@kenneth-marut-work do you mind rebasing, and I'd be happy to take a look 👍 As for the size of the view, it doesn't need to be part of this pull-request, I believe all tree-view parts suffer from the same bug where the size is not controllable.

vince-fugnitto · 2021-07-09T18:00:19Z

packages/editor-preview/package.json

+    "@theia/editor": "1.15.0",
+    "@theia/navigator": "1.15.0",
+    "@theia/workspace": "1.15.0"


@kenneth-marut-work @colin-grant-work is the new coupling necessary?
@theia/editor only uses core, why does editor-preview need the coupling?

The two of us discussed this since the tree decorator (which is responsible for italicizing the filename when in preview mode, and also adding workspace/tail decorations) was decided to be moved from the navigator -> editor preview package. This allows the decorator to have access to the editor-preview's built-in checks for preview state, and emitters for on-preview-pinned.

It has the sideeffect of needing to depend on the navigator package (to access the decorator service) as well as the workspace service. The SCM and markers package have a similar dependency set up.

packages/navigator/src/browser/open-editors-widget/navigator-open-editors-commands.ts

vince-fugnitto · 2021-07-12T13:10:03Z

packages/navigator/src/browser/open-editors-widget/open-editors.css

+    font-size: var(--theia-ui-font-size0);
+}
+
+.open-editors-node-row .open-editors-prefix-icon-container {


I believe we should prefix the new stylings with theia-open-editors-widget

vince-fugnitto · 2021-07-12T13:17:41Z

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

+                const widgets = openEditorsWidget.getEditorWidgetsByGroup(id);
+                widgets?.forEach(widget => widget.close());
+            },
+            isVisible: () => false


To confirm, the following is set to false to hide it from the command palette?

So these two commands I've decided to hide from the command palette since they are essentially toolbar icon commands. Their implementation is slightly different from the way VSCode would implement them as they accept either an ApplicationShell.Area or TabBar<Widget> command as their argument.

I thought a simpler way would be to not give it a label so it does not show up in the command palette?
Although, I'm fine with you explicitly setting it to false as well.

vince-fugnitto · 2021-07-12T13:19:27Z

packages/navigator/src/browser/open-editors-widget/navigator-open-editors-commands.ts

+
+export namespace OpenEditorsCommands {
+    export const CLOSE_ALL_TABS_FROM_TOOLBAR: Command = {
+        id: 'navigator.close.all.editors',


For the new commands in the file, should we align their ids with that of vscode so the plugin-system can directly reference them without the mapping?

See note above, I've kept these commands separate from the plugin commands since their implementation is slightly different in both arguments and behavior. I've added added an additional command with VSCode's ID to the plugin-vscode-commands-contribution for workbench.files.saveAllInGroup which uses a very similar implementation as the existing workbench.action.closeEditorsInGroup and ensured that the two now have labels so that they can be accessed from the command palette. It's a bit tricky to figure out exactly where some of these things should live, but this was my best attempt at separating responsibilities. I've also added an appropriate utility in the application shell for saving editors which I am using in the execute of the toolbar commands.

Let me know if you have suggestions, thanks!

vince-fugnitto · 2021-07-15T18:00:33Z

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

@@ -105,7 +113,8 @@ export namespace FileNavigatorCommands {
        label: 'Focus on Files Explorer'
    };
    export const COPY_RELATIVE_FILE_PATH: Command = {
-        id: 'navigator.copyRelativeFilePath'
+        id: 'navigator.copyRelativeFilePath',
+        label: 'Copy Relative Path'


Should we align?

Suggested change

label: 'Copy Relative Path'

label: 'Copy Relative Path of Active File'

In VSCode, when I right click on files and look for this command I see "Copy Relative Path". I do see two labels for this command in the VSCode repo, but I'm not sure I understand how VSCode represents their commands to make the call:

https://github.com/microsoft/vscode/blob/2853fd0ffcc4c067bf7b5b31ccfc15ac245fc89c/src/vs/workbench/contrib/files/browser/fileActions.contribution.ts#L163

https://github.com/microsoft/vscode/blob/2853fd0ffcc4c067bf7b5b31ccfc15ac245fc89c/src/vs/workbench/contrib/files/browser/fileActions.contribution.ts#L215

vince-fugnitto · 2021-07-15T18:03:56Z

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

+            isVisible: widget => this.withOpenEditorsWidget(widget, () => !!this.editorWidgets.length)
+        });
+        registry.registerCommand(OpenEditorsCommands.SAVE_ALL_TABS_FROM_TOOLBAR, {
+            execute: widget => this.withOpenEditorsWidget(widget, () => this.commandService.executeCommand(CommonCommands.SAVE_ALL.id)),


No need for the CommandService injection:

Suggested change

execute: widget => this.withOpenEditorsWidget(widget, () => this.commandService.executeCommand(CommonCommands.SAVE_ALL.id)),

execute: widget => this.withOpenEditorsWidget(widget, () => registry.executeCommand(CommonCommands.SAVE_ALL.id)),

Good to know, I've fixed this 👍

vince-fugnitto · 2021-07-15T18:08:19Z

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

+                const widgets = openEditorsWidget.getEditorWidgetsByGroup(id);
+                widgets?.forEach(widget => widget.close());
+            },
+            isVisible: () => false


I thought a simpler way would be to not give it a label so it does not show up in the command palette?
Although, I'm fine with you explicitly setting it to false as well.

vince-fugnitto · 2021-07-15T18:09:42Z

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

@@ -412,7 +463,7 @@ export class FileNavigatorContribution extends AbstractViewContribution<FileNavi
        });
        registry.registerMenuAction(NavigatorContextMenu.CLIPBOARD, {
            commandId: FileNavigatorCommands.COPY_RELATIVE_FILE_PATH.id,
-            label: 'Copy Relative Path',
+            label: FileNavigatorCommands.COPY_RELATIVE_FILE_PATH.label,


I believe it can be omitted if the label is the same as the command definition.
The same is true for other occurences.

Also good to know, I've removed the labels from the menu items

vince-fugnitto · 2021-07-15T18:16:12Z

packages/navigator/src/browser/open-editors-widget/navigator-open-editors-decorator-service.ts

+export const OpenEditorsTreeDecorator = Symbol('OpenEditorsTreeDecorator');
+
+@injectable()
+export class OpenEditorsTreeDecoratorService extends AbstractTreeDecoratorService {


I'm a bit confused as to this file.

why does it live in navigator, navigator should not know about problems.

I believe we want a open-editor decorator service which problems contributes to, like the following in problems/.

bind(NavigatorTreeDecorator).toService(ProblemDecorator);

I've moved the suffix decorations that add the workspace * path/to/file to the problem decorator and added a conditional check if the decorated node belongs to the OpenEditorsWidget. This way the suffix decorations are only applied to open-editor nodes and not the navigator nodes. Moving that logic allows this file to be much less complex :)

Thanks for the suggestions and help with this issue!

The only downside is that I cannot find a way to aggregate the italicization provided by the editor-preview-tree-decorator into the suffix decoration, which causes a preview widget node to not have italicization on its suffix. I don't think this is really a problem and the cleaner code outweighs the minor UI flaw. This could also be addressed in a separate PR

kenneth-marut-work · 2021-07-16T21:49:17Z

@vince-fugnitto thanks for the helpful suggestions. I believe I have addressed your comments except for the minor issue with the visible: () => false check which I've decided to keep as I refer to those commands' labels by their Variable name in the OpenEditorsWidget for convenience.

Moving the suffix-decorations into the problem-decorator seemed to work quite well and greatly simplified the strange logic I was doing in the open-editors-decorator service to retroactively apply coloring, so now that file is much more straightforward.

kenneth-marut-work · 2021-07-20T15:05:09Z

@vince-fugnitto thanks for pointing out the path display issue. I've fixed this and also added a case for when a file is opened outside a workspace (in that case we display the full path). This works for the case of no-workspace-open case as well:

Single root with nested files and file outside workspace

Multiroot with nested files, and file outside workspace

No-root workspace with file

vince-fugnitto

The changes work well for me, and the code is also clean 👍

I attempted to review multiple use-cases:

The changes are already substantial, if there are improvements we can make them in subsequent pull-requests 👍

kenneth-marut-work · 2021-07-20T16:20:32Z

Signed-off-by: Kenneth Marut <[email protected]>

kenneth-marut-work force-pushed the feature/open-editors-widget branch 2 times, most recently from 84f046f to 71be806 Compare April 2, 2021 20:39

kenneth-marut-work changed the title ~~WIP: Open editors widget in navigator~~ Open editors widget in navigator Apr 2, 2021

kenneth-marut-work requested a review from colin-grant-work April 2, 2021 20:44

colin-grant-work reviewed Apr 5, 2021

View reviewed changes

kenneth-marut-work force-pushed the feature/open-editors-widget branch from 71be806 to c543684 Compare April 5, 2021 19:39

kenneth-marut-work marked this pull request as ready for review April 5, 2021 19:40

kenneth-marut-work force-pushed the feature/open-editors-widget branch from c543684 to e4cc5d3 Compare April 5, 2021 19:42

vince-fugnitto self-requested a review April 6, 2021 14:58

vince-fugnitto added enhancement issues that are enhancements to current functionality - nice to haves navigator issues related to the navigator/explorer labels Apr 6, 2021

colin-grant-work reviewed Apr 16, 2021

View reviewed changes

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

kenneth-marut-work force-pushed the feature/open-editors-widget branch 2 times, most recently from 5826303 to c8ca0ba Compare April 22, 2021 15:08

vince-fugnitto reviewed Apr 22, 2021

View reviewed changes

packages/navigator/package.json Outdated Show resolved Hide resolved

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

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

kenneth-marut-work force-pushed the feature/open-editors-widget branch from c8ca0ba to e7828b9 Compare April 23, 2021 16:57

colin-grant-work reviewed May 6, 2021

View reviewed changes

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

colin-grant-work reviewed May 10, 2021

View reviewed changes

packages/navigator/src/browser/navigator-widget-factory.ts Show resolved Hide resolved

kenneth-marut-work force-pushed the feature/open-editors-widget branch from e7828b9 to 093b36e Compare May 12, 2021 19:57

kenneth-marut-work mentioned this pull request May 14, 2021

Race condition in Editor Preview Manager and Editor Manager #9485

Closed

kenneth-marut-work force-pushed the feature/open-editors-widget branch from 093b36e to d83370a Compare June 30, 2021 18:38

colin-grant-work self-requested a review July 2, 2021 14:34

colin-grant-work reviewed Jul 2, 2021

View reviewed changes

colin-grant-work reviewed Jul 8, 2021

View reviewed changes

packages/editor-preview/src/browser/editor-preview-tree-decorator.ts Outdated Show resolved Hide resolved

packages/editor-preview/src/browser/editor-preview-tree-decorator.ts Outdated Show resolved Hide resolved

colin-grant-work approved these changes Jul 8, 2021

View reviewed changes

kenneth-marut-work force-pushed the feature/open-editors-widget branch from 223eb65 to 07d1412 Compare July 9, 2021 17:35

vince-fugnitto reviewed Jul 9, 2021

View reviewed changes

vince-fugnitto reviewed Jul 12, 2021

View reviewed changes

kenneth-marut-work force-pushed the feature/open-editors-widget branch 3 times, most recently from 62ad33a to 98a21a7 Compare July 14, 2021 21:49

vince-fugnitto reviewed Jul 15, 2021

View reviewed changes

kenneth-marut-work force-pushed the feature/open-editors-widget branch from 98a21a7 to 48593f3 Compare July 16, 2021 21:36

vince-fugnitto approved these changes Jul 20, 2021

View reviewed changes

Feature: Open editors widget in navigator

a0472f6

Signed-off-by: Kenneth Marut <[email protected]>

kenneth-marut-work force-pushed the feature/open-editors-widget branch from 9e56780 to a0472f6 Compare July 20, 2021 16:42

kenneth-marut-work merged commit f7d1a2d into eclipse-theia:master Jul 21, 2021

vince-fugnitto added this to the 1.16.0 milestone Jul 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Open editors widget in navigator #9284

Open editors widget in navigator #9284

kenneth-marut-work commented Mar 30, 2021 •

edited

Loading

colin-grant-work left a comment

vince-fugnitto left a comment

kenneth-marut-work commented Apr 23, 2021

colin-grant-work left a comment

kenneth-marut-work commented May 12, 2021

kenneth-marut-work commented Jun 30, 2021

colin-grant-work left a comment

colin-grant-work left a comment

kenneth-marut-work commented Jul 9, 2021

vince-fugnitto commented Jul 9, 2021

vince-fugnitto Jul 9, 2021

kenneth-marut-work Jul 9, 2021

vince-fugnitto Jul 12, 2021

kenneth-marut-work Jul 14, 2021

vince-fugnitto Jul 12, 2021

kenneth-marut-work Jul 14, 2021

vince-fugnitto Jul 15, 2021

vince-fugnitto Jul 12, 2021

kenneth-marut-work Jul 14, 2021

vince-fugnitto Jul 15, 2021

kenneth-marut-work Jul 15, 2021

vince-fugnitto Jul 15, 2021

kenneth-marut-work Jul 16, 2021

vince-fugnitto Jul 15, 2021

vince-fugnitto Jul 15, 2021

kenneth-marut-work Jul 16, 2021

vince-fugnitto Jul 15, 2021

kenneth-marut-work Jul 16, 2021

kenneth-marut-work commented Jul 16, 2021

kenneth-marut-work commented Jul 20, 2021

vince-fugnitto left a comment

kenneth-marut-work commented Jul 20, 2021

	label: 'Copy Relative Path'
	label: 'Copy Relative Path of Active File'

	execute: widget => this.withOpenEditorsWidget(widget, () => this.commandService.executeCommand(CommonCommands.SAVE_ALL.id)),
	execute: widget => this.withOpenEditorsWidget(widget, () => registry.executeCommand(CommonCommands.SAVE_ALL.id)),

Open editors widget in navigator #9284

Open editors widget in navigator #9284

Conversation

kenneth-marut-work commented Mar 30, 2021 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

colin-grant-work left a comment

Choose a reason for hiding this comment

vince-fugnitto left a comment

Choose a reason for hiding this comment

kenneth-marut-work commented Apr 23, 2021

colin-grant-work left a comment

Choose a reason for hiding this comment

kenneth-marut-work commented May 12, 2021

kenneth-marut-work commented Jun 30, 2021

colin-grant-work left a comment

Choose a reason for hiding this comment

colin-grant-work left a comment

Choose a reason for hiding this comment

kenneth-marut-work commented Jul 9, 2021

vince-fugnitto commented Jul 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenneth-marut-work commented Jul 16, 2021

kenneth-marut-work commented Jul 20, 2021

vince-fugnitto left a comment

Choose a reason for hiding this comment

kenneth-marut-work commented Jul 20, 2021

kenneth-marut-work commented Mar 30, 2021 •

edited

Loading