Navigator:Select Folder when user creates one. #6927

Anasshahidd21 · 2020-01-20T22:44:16Z

What it does

Problem Before: When you create a new folder, the folder is not selected. However, in VSCode it is.
Solution: Updated the Workspace service to contain the createFile and createFolder methods, which would call the filesystem to create it for us, then an event is fired so that the navigator-widget would know about it, once the file receives the uri. It will then create a URI, get the nodes by that URI, and then select that specific node.

Issue ID: 6190

Signed-off-by: Muhammad Anas Shahid

How to test

Create a new folder, checks to see if the folder is highlighted.
Create a new file, check to see if the file is highlighted and opened up as well.

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

Thank you for your first contribution 👍

Please be sure to properly sign the Eclipse ECA so that the project can accept your changes (it is the reason for the failed CI check).

In the future, you can use a keyword to automatically closed referenced issues when a pull-request is merged (ex: Fixes #6190))

vince-fugnitto · 2020-01-20T23:42:58Z

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

@@ -25,7 +25,6 @@ import { FileNavigatorFilter } from './navigator-filter';
 export class FileNavigatorTree extends FileTree {

    @inject(FileNavigatorFilter) protected readonly filter: FileNavigatorFilter;
-


@Anasshahidd21 can you revert changes made to the following file? (they are unnecessary)

vince-fugnitto · 2020-01-20T23:48:01Z

packages/workspace/src/browser/workspace-service.ts

-
+    protected readonly onDidCreateFileFolderEmitter = new Emitter<string>();
+    readonly onDidCreateFileFolder = this.onDidCreateFileFolderEmitter.event;
+    readonly onDidReveal = new Emitter<void>().event;


[minor] include a newline so it's easy to read.

[minor] it'd be nice to include documentation describing the events

vince-fugnitto · 2020-01-20T23:48:20Z

packages/workspace/src/browser/workspace-service.ts

-
+    protected readonly onDidCreateFileFolderEmitter = new Emitter<string>();
+    readonly onDidCreateFileFolder = this.onDidCreateFileFolderEmitter.event;
+    readonly onDidReveal = new Emitter<void>().event;


onDidReveal looks to be a leftover as it is not used.

vince-fugnitto · 2020-01-20T23:49:18Z

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

            })
        ]);
    }
-


[minor] preserve the newline, as it makes methods easier to read.

akosyakov · 2020-01-21T02:38:44Z

packages/workspace/src/browser/workspace-service.ts

@@ -66,7 +66,9 @@ export class WorkspaceService implements FrontendApplicationContribution {
    protected readonly schemaProvider: PreferenceSchemaProvider;

    protected applicationName: string;
-
+    protected readonly onDidCreateFileFolderEmitter = new Emitter<string>();


I'm not sure that the workspace service should have such APIs.

So if someone call create a folder programatically, this file will be always revealed in the navigator? Is it desirable? I think the scope of the change should be bound to the navigator.

There are already APIs btw to listen when something is created: FileSystemWatcher. And it should be used.

But we don't want to highlight any folder that gets created. Right now creating a folder in the navigator tree triggers a command from the workspace extension. How would you fix #6190 ?

But we don't want to highlight any folder that gets created.

Changing WorkspaceService does not help, anyone can trigger programmatically these methods now and it would lead to the same issues.

How would you fix #6190 ?

If it should be triggered only by user actions then it should be implemented there, like await then create finished (without listening to events), if a result is positive then reveal. WorkspaceService should not be changed.

Problem Before: When you create a new folder, the folder is not selected. However, in VSCode it is. Solution: Updated the Workspace service to contain the createFile and createFolder methods, which would call the filesystem to create it for us, then an event is fired so that the navigator-widget would know about it, once the file receives the uri. It will then create a URI, get the nodes by that URI, and then select that specific node. Issue ID: 6190 Signed-off-by: Muhammad Anas Shahid <muhammad.shahid@ericsson.com>

vince-fugnitto · 2020-01-27T16:39:53Z

I created a small POC of how we can support the changes without the need for new methods in the workspace-service an the new event.

In this way, we create a new command CREATE_FILE_AND_REVEAL which internally calls the WorkspaceCommands.NEW_FILE (for the dialog and file creation) and also adds extra handling to reveal the file in the explorer. I assume this would be a straightforward way to work around the issues we were initially having with our approaches.

akosyakov · 2020-01-28T03:10:43Z

In this way, we create a new command CREATE_FILE_AND_REVEAL which internally calls the WorkspaceCommands.NEW_FILE (for the dialog and file creation) and also adds extra handling to reveal the file in the explorer. I assume this would be a straightforward way to work around the issues we were initially having with our approaches.

@vince-fugnitto Maybe the navigator could provide an alternative handler for the same command when it is a current widget, in such case it would delegate to the original handler and then reveal. So it works from any context given that the navigator was last focused widget.

vince-fugnitto · 2020-01-28T11:36:52Z

In this way, we create a new command CREATE_FILE_AND_REVEAL which internally calls the WorkspaceCommands.NEW_FILE (for the dialog and file creation) and also adds extra handling to reveal the file in the explorer. I assume this would be a straightforward way to work around the issues we were initially having with our approaches.

@vince-fugnitto Maybe the navigator could provide an alternative handler for the same command when it is a current widget, in such case it would delegate to the original handler and then reveal. So it works from any context given that the navigator was last focused widget.

I wasn't aware we could register multiple alternative handlers for a given command (I haven't seen an example yet of such). If so, I think it'd be a good approach :)

akosyakov · 2020-01-28T11:39:30Z

I wasn't aware we could register multiple alternative handlers for a given command (I haven't seen an example yet of such). If so, I think it'd be a good approach :)

copy/paste have for example

vince-fugnitto · 2020-01-28T12:17:45Z

I wasn't aware we could register multiple alternative handlers for a given command (I haven't seen an example yet of such). If so, I think it'd be a good approach :)

copy/paste have for example

I was unable to identify the alternative handlers for copy/paste :(

akosyakov · 2020-01-29T02:44:43Z

@vince-fugnitto try to register another handler for the same command but with different isEnabled implementation, it could be that we should reverse evaluation of command handlers that added later have a chance. In this case they should be more specific. Maybe we have to add such docs to registerHandler

Anasshahidd21 force-pushed the anas/ui branch from 52d79e9 to e22c55d Compare January 20, 2020 22:53

vince-fugnitto reviewed Jan 20, 2020

View reviewed changes

vince-fugnitto added enhancement issues that are enhancements to current functionality - nice to haves navigator issues related to the navigator/explorer ui/ux issues related to user interface / user experience labels Jan 20, 2020

akosyakov reviewed Jan 21, 2020

View reviewed changes

Anasshahidd21 force-pushed the anas/ui branch from e22c55d to a9dcfbf Compare January 21, 2020 14:37

Anasshahidd21 force-pushed the anas/ui branch from a9dcfbf to e32250e Compare January 21, 2020 14:41

vinokurig mentioned this pull request May 6, 2020

Select created new files and folders in the navigator #7762

Merged

1 task

vinokurig closed this in #7762 May 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Navigator:Select Folder when user creates one. #6927

Navigator:Select Folder when user creates one. #6927

Anasshahidd21 commented Jan 20, 2020 •

edited

Loading

vince-fugnitto left a comment

vince-fugnitto Jan 20, 2020

vince-fugnitto Jan 20, 2020

vince-fugnitto Jan 20, 2020

vince-fugnitto Jan 20, 2020

akosyakov Jan 21, 2020

akosyakov Jan 21, 2020

paul-marechal Jan 21, 2020

akosyakov Jan 22, 2020

vince-fugnitto commented Jan 27, 2020

akosyakov commented Jan 28, 2020

vince-fugnitto commented Jan 28, 2020

akosyakov commented Jan 28, 2020

vince-fugnitto commented Jan 28, 2020

akosyakov commented Jan 29, 2020

		@@ -25,7 +25,6 @@ import { FileNavigatorFilter } from './navigator-filter';
		export class FileNavigatorTree extends FileTree {

		@inject(FileNavigatorFilter) protected readonly filter: FileNavigatorFilter;

Navigator:Select Folder when user creates one. #6927

Navigator:Select Folder when user creates one. #6927

Conversation

Anasshahidd21 commented Jan 20, 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

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

vince-fugnitto commented Jan 27, 2020

akosyakov commented Jan 28, 2020

vince-fugnitto commented Jan 28, 2020

akosyakov commented Jan 28, 2020

vince-fugnitto commented Jan 28, 2020

akosyakov commented Jan 29, 2020

Anasshahidd21 commented Jan 20, 2020 •

edited

Loading