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

Select created new files and folders in the navigator #7762

Merged
merged 1 commit into from
May 14, 2020
Merged

Conversation

vinokurig
Copy link
Contributor

@vinokurig vinokurig commented May 6, 2020

What it does

Add an event that fires when new nodes added to tree.
Select new created files and folders.

fixes #6190
closes #6927

How to test

  1. Create new file/folder => the file/folder must be selected.
  2. Create new file/folder in selected directory => the file/folder must be selected.
    video-convert

Review checklist

Reminder for reviewers

@vince-fugnitto vince-fugnitto added the navigator issues related to the navigator/explorer label May 6, 2020
@akosyakov
Copy link
Member

I don't think it is a good idea, programatically new nodes can be added for all kind of reasons. It does not mean that they should be selected. I don't think we need to change anything in trees, but rather new file/folder commands that they select a node after createFile/Folder requests are done.

@vinokurig
Copy link
Contributor Author

@akosyakov We still need to wait for the new nodes to be added to the tree, so I've added a new event that fired when the new file/folder action is executed and check if the added node was caused by the action.

@vince-fugnitto
Copy link
Member

@akosyakov We still need to wait for the new nodes to be added to the tree, so I've added a new event that fired when the new file/folder action is executed and check if the added node was caused by the action.

It is similar to the initial implementation of #6927 (comment)

@vinokurig
Copy link
Contributor Author

It is similar to the initial implementation of #6927 (comment)

My implementation is a little bit different: the workspace command events are fired directly from the new file/folder actions.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

Please see the CI errors related to the test failures.

@vinokurig
Copy link
Contributor Author

@vince-fugnitto

Please see the CI errors related to the test failures.

done

@akosyakov
Copy link
Member

I don't understand why need events, I imagine code like that:

await FileSystem.createFile(uri);
await FileNavigatorWidget.model.refresh(uri.parent);
await FileNavigatorWidget.revealFile(uri); // maybe previous step is not necessary, if `revealFile` does refresh already

@akosyakov akosyakov added the workspace issues related to the workspace label May 12, 2020
@vinokurig
Copy link
Contributor Author

@akosyakov

I don't understand why need events, I imagine code like that:
await FileSystem.createFile(uri);
await FileNavigatorWidget.model.refresh(uri.parent);
await FileNavigatorWidget.revealFile(uri); // maybe previous step is not necessary, if revealFile does refresh already

I couldn't use navigator in the workspace module because workspace is already used in the navigator, so injecting navigator to workspace causes circular dependency error.

@akosyakov
Copy link
Member

I couldn't use navigator in the workspace module because workspace is already used in the navigator, so injecting navigator to workspace causes circular dependency error.

But you can do it otherwise by injecting WorkspaceCommandContribution into FileNavigatorContribution and then:

this.workspaceContribution.onDidCreateNewFolder(uri => {
    const navigator = this.tryGetWidget();
    if (navigator) {
        const parent = await navigator.model.revealFile(uri.parent);
        if (CompositeTreeNode.is(parent)) {
            await navigator.model.refresh(parent);
            const node = await navigator.model.revealFile(uri);
            if (SelectableTreeNode.is(node)) {
                navigator.model.selectNode(node);
            }
        }
    }
});

@vinokurig vinokurig force-pushed the theia-6190 branch 2 times, most recently from 2529b47 to b432bdb Compare May 13, 2020 09:27
@vinokurig
Copy link
Contributor Author

@akosyakov

But you can do it otherwise by injecting WorkspaceCommandContribution into FileNavigatorContribution and then:

   const navigator = this.tryGetWidget();
   if (navigator) {
        const parent = await navigator.model.revealFile(uri.parent);
        if (CompositeTreeNode.is(parent)) {
            await navigator.model.refresh(parent);
            const node = await navigator.model.revealFile(uri);
            if (SelectableTreeNode.is(node)) {
                navigator.model.selectNode(node);
            }
        }
    }
});

done

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

It worked nicely. I've tested:

  • with and without navigator (from command palette)
  • creating files and folders
  • creates with simple name and nested structures (foo/bar/baz)

@akosyakov
Copy link
Member

did a quick test, still looks good, squash and merge :)

Signed-off-by: Igor Vinokur <ivinokur@redhat.com>
@vinokurig vinokurig merged commit 8b18b5a into master May 14, 2020
@vinokurig vinokurig deleted the theia-6190 branch May 14, 2020 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
navigator issues related to the navigator/explorer workspace issues related to the workspace
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Created file/folder should be selected
3 participants