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

[explorer] implement 'more' toolbar item for the explorer #5953

Merged
merged 1 commit into from
Aug 26, 2019

Conversation

vince-fugnitto
Copy link
Member

@vince-fugnitto vince-fugnitto commented Aug 15, 2019

What it does

Fixes #5951
Fixes #6010

  • Added a new toolbar item for the explorer which consists of mutliple commands.
  • Adjusted the WorkspaceRootUriAwareCommandHandler to better handle commands which
    can be used by the workspace root in both a single and multi-root workspace.
  • Added the command New File to the toolbar item.
  • Added the command New Folder to the toolbar item.
  • Added the command Add Folder to Workspace... to the toolbar item.
  • Added the command Compare With... to the toolbar item.

Available Toolbar Item Commands

  • New File
  • New Folder
  • Add Folder to Workspace...
  • Compare With...
Screen Shot 2019-08-21 at 3 40 20 PM

How to test

  1. test the following commands in a single-root workspace
    1.1. New File: creates a new file at the root if no selection is present, else it creates it at the selection.
    1.2. New Folder: creates a new folder at the root if no selection is present, else it creates it at the selection
    1.3. Add Folder to Workspace...: adds a folder to the workspace
    1.4 Compare With...: if no selection it uses the root, else it uses the selection. prompts the user to select a branch to compare to

  2. test the following commands in a multi-root workspace (with roots)
    2.1. New File: creates a new file at the first root if no selection is present, else it creates it at the selection.
    2.2. New Folder: creates a new folder at the first root if no selection is present, else it creates it at the selection
    2.3. Add Folder to Workspace...: adds a folder to the workspace

  3. test the following command in a multi-root workspace (without any roots)
    3.1 Add Folder to Workspace...: adds a folder to the workspace

Review checklist

Single-Root Workspace

Command Toolbar Selection
New File
New Folder
Add Folder to Workspace...
Compare With...

Multi-Root Workspace

Command Toolbar Selection
New File
(adds to first root)
New Folder
(adds to first root)
Add Folder to Workspace...
Compare With...

Multi-Root Workspace (no roots)

Screen Shot 2019-08-21 at 4 07 50 PM

Command Toolbar Selection
Add Folder to Workspace...

Reminder for reviewers

Signed-off-by: Vincent Fugnitto [email protected]

@vince-fugnitto vince-fugnitto added enhancement issues that are enhancements to current functionality - nice to haves navigator issues related to the navigator/explorer labels Aug 15, 2019
@vince-fugnitto vince-fugnitto self-assigned this Aug 15, 2019
@vince-fugnitto
Copy link
Member Author

I'm blocked for certain commands, example Find in Folder. The command currently exists in the @theia/search-in-workspace extension and it is currently added to the navigator through the context menu NavigatorContextMenu.SEARCH. I'm not sure if its possible to expose the more toolbar item to accept contributions from other extensions and mimic the behavior of the entire context menu for the root.

@akosyakov
Copy link
Member

@theia/search-in-workspace has a depdency to @theia/navigator, so you can register toolbar item for the navigator in SearchInWorkspaceFrontendContribution.

@vince-fugnitto
Copy link
Member Author

@theia/search-in-workspace has a depdency to @theia/navigator, so you can register toolbar item for the navigator in SearchInWorkspaceFrontendContribution.

I wasn't aware that other extensions can contribute to the toolbar items defined in a different extension. Must I expose it somehow?

@akosyakov
Copy link
Member

@vince-fugnitto I'm not sure what you want to expose? The registry of toolbar items is global. Any extension can register item and then do an instance check whether the current widget is a navigator or not for isEnabled and isVisible callback of an underlying toolbar item command.

@vince-fugnitto
Copy link
Member Author

@vince-fugnitto I'm not sure what you want to expose? The registry of toolbar items is global. Any extension can register item and then do an instance check whether the current widget is a navigator or not for isEnabled and isVisible callback of an underlying toolbar item command.

Sorry I was just confused if it was possible to contribute to the same toolbar item, confused as to how it should be referenced from another extension.

@vince-fugnitto vince-fugnitto force-pushed the vf/GH-5951 branch 4 times, most recently from e8cb6f6 to e58e858 Compare August 20, 2019 17:37
@vince-fugnitto vince-fugnitto marked this pull request as ready for review August 20, 2019 17:38
@vince-fugnitto vince-fugnitto requested a review from lmcbout August 20, 2019 17:38
@lmcbout
Copy link
Contributor

lmcbout commented Aug 20, 2019

The current menu item generates the correct behavior. Nevertheless, I think the following commands should also be available from the menu as a minimum:

  • Compare With...
    If the pref "workspace.supportMultiRootWorkspace" is available then
    • Add Folder to workspace
    • Remove folder from Workspace

I noticed that if I add a second root workspace, then remove it, the "..." in the menu bar does not come back, so unable to select any commands anymore

@vince-fugnitto
Copy link
Member Author

Updated to include:

  • Add Folder to Workspace
  • Compare With...

@vince-fugnitto
Copy link
Member Author

vince-fugnitto commented Aug 20, 2019

@lmcbout

  • Compare With...

Added.

If the pref "workspace.supportMultiRootWorkspace" is available then

  • Add Folder to workspace
  • Remove folder from Workspace

I only display Add Folder to Workspace if there is a single-root workspace present.
In the case of multi-root the default behavior is used. Select roots to add / remove.

I noticed that if I add a second root workspace, then remove it, the "..." in the menu bar does not come back, so unable to select any commands anymore.

The toolbar is only displayed in a single-root workspace (no root node is visible in the tree).
For multi-root the default behavior should be used.

@vince-fugnitto vince-fugnitto changed the title Implement 'more' toolbar item for the explorer wip: [explorer] implement 'more' toolbar item for the explorer Aug 21, 2019
@vince-fugnitto vince-fugnitto force-pushed the vf/GH-5951 branch 2 times, most recently from 9d73b7d to 811e6c1 Compare August 21, 2019 19:43
@vince-fugnitto
Copy link
Member Author

@akosyakov @lmcbout @svenefftinge
Are there any additional commands which should be added in the initial implentation?

@vince-fugnitto vince-fugnitto changed the title wip: [explorer] implement 'more' toolbar item for the explorer [explorer] implement 'more' toolbar item for the explorer Aug 22, 2019
@lmcbout
Copy link
Contributor

lmcbout commented Aug 22, 2019

The diff I am looking for is this current diff against master, I should have 4 files changed, but get nothing.

Diff5951WithMaster

@vince-fugnitto
Copy link
Member Author

The diff I am looking for is this current diff against master, I should have 4 files changed, but get nothing.

Diff5951WithMaster

@lmcbout
If you look at your .gif, you are selecting .theia which likely has no changes.

@lmcbout
Copy link
Contributor

lmcbout commented Aug 22, 2019

@vince-fugnitto Ok, so If I collapse the workspace, there is no "..." to select and get to the "Compare with..." command. The minute I select any folder, impossible to select the "Compare with for the workspace ?
DiffCollapse

@vince-fugnitto
Copy link
Member Author

@vince-fugnitto Ok, so If I colalpse the workspace, there is no "..." to select and get to the "Compare with..." command. The minute I select any folder, impossible to select the "Compare with for the workspace ?

Correct, that is the proper implementation of panels in a view container.
When collapsed, all toolbar items (including mine) are not visible.

Verified in VSCode:
a

@lmcbout
Copy link
Contributor

lmcbout commented Aug 22, 2019

So, how to get to the "Compare with..." command if there has been a selection of file/folder and I want to compare the whole branch, not just a sub-folder ?

@vince-fugnitto
Copy link
Member Author

So, how to get to the "Compare with..." command if there has been a selection of file/folder and I want to compare the whole branch, not just a sub-folder ?

You can always un-select your selection, similarly to how you un-select in a native desktop dialog.

Example: (using cmd+click on OSX)

d

@lmcbout
Copy link
Contributor

lmcbout commented Aug 22, 2019

You can always un-select your selection, similarly to how you un-select in a native desktop dialog.

I was not aware we could un-select on Ubuntu, (CTRL + mouse left click)
Did not find any documentation either about it
Since it is not a known command, we should have something to let the user know either:

  • How to un-select a file/ folder in his workspace,
  • Have a command to un-select the current selection (should be intuitive)
  • Have another command "Compare workspace with" which select the root folder before doing a "Compare with..." command

@vince-fugnitto
Copy link
Member Author

I was not aware we could un-select on Ubuntu, (CTRL + mouse left click)

I believe it's pretty standard across all operating systems, similar to multi-select.

Since it is not a known command, we should have something to let the user know either:

I disagree, it's pretty well known how to select, un-select and multi-select.

  • How to un-select a file/ folder in his workspace,

I guess it can be added in a general user guide in the future.
Currently we do not have a user guide at all.

  • Have a command to un-select the current selection (should be intuitive)

I think it'd be overkill to have such commands when it can easily be un-selected with a click.

  • Have another command "Compare workspace with" which select the root folder before doing a "Compare with..." command

The command Compare With... compares the branch history if no selection is present, else it compares the folder history. Initially I had a command solely for the root of the workspace but did not want to duplicate the command unnecessarily.

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 works nicely, but the code needs clean up.

@akosyakov
Copy link
Member

@vince-fugnitto I think it is important to have this PR done for 0.10.0. If you can please make it your priority to wrap it up.

@vince-fugnitto
Copy link
Member Author

@akosyakov I've updated the code based on your comments :)

@vince-fugnitto
Copy link
Member Author

Thank you for the feedback @akosyakov, I now updated the code and also pass args to isEnabled, isVisible and getUri.

@akosyakov
Copy link
Member

@vince-fugnitto please have a look again

Fixes #5951
Fixes #6010

- Added a new toolbar item for the `explorer` which consists of multiple commands.
- Adjusted the `WorkspaceRootUriAwareCommandHandler` to better handle commands which
can be used by the workspace root in both a single and multi-root workspace.
- Added the command `New File` to the toolbar item.
- Added the command `New Folder` to the toolbar item.
- Added the command `Add Folder to Workspace...` to the toolbar item.
- Added the command `Compare With...` to the toolbar item.

Signed-off-by: Vincent Fugnitto <[email protected]>
@vince-fugnitto
Copy link
Member Author

vince-fugnitto commented Aug 25, 2019

@akosyakov are there any other changes required before merging?

@akosyakov
Copy link
Member

@vince-fugnitto code looks good to me now, i gave a quick test and it works well also

@akosyakov
Copy link
Member

@vince-fugnitto please merge, as soon as you are online

@vince-fugnitto vince-fugnitto merged commit e4390f4 into master Aug 26, 2019
@vince-fugnitto
Copy link
Member Author

Thank you @akosyakov for the help and review :)

@vince-fugnitto vince-fugnitto deleted the vf/GH-5951 branch August 26, 2019 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement issues that are enhancements to current functionality - nice to haves navigator issues related to the navigator/explorer
Projects
None yet
3 participants