Add context menu to search tree #46311
Conversation
|
Here's some feedback after trying it out:
Regarding code I will comment directly in the code. |
| @@ -474,8 +474,10 @@ export abstract class AbstractSearchAndReplaceAction extends Action { | |||
|
|
|||
There was a problem hiding this comment.
The proper way to do all this action command business is the following:
- Have a command that is self contained and does the actual work
- Use the command everywhere except Action Bar
- For the Action Bar instatiate an Action that will only use the commandService to execute the command
That patter is cleanest and we use it all over the place (especially explorer).
If you do not want to tackle it in this PR then you can create a debt item.
There was a problem hiding this comment.
I'll create an item to convert search Actions to commands
| @@ -354,3 +359,63 @@ export class SearchFilter implements IFilter { | |||
| return !(element instanceof FileMatch || element instanceof FolderMatch) || element.matches().length > 0; | |||
| } | |||
| } | |||
|
|
|||
| export class SearchTreeController extends WorkbenchTreeController { | |||
There was a problem hiding this comment.
This works fine and we have it for instance in debug land.
However this is an old way to do this - it is a sort of bridge between comands and actions.
Since all your things could be commands this can than be much simpliar without any action items and things like that.
I suggest to look here for an inspiration on how to do it if all you guys are commands
https://github.com/Microsoft/vscode/blob/roblou/searchContextMenu/src/vs/workbench/parts/files/electron-browser/views/openEditorsView.ts#L312
There was a problem hiding this comment.
I'm guessing you're referring to what I do inside showContextMenu, and not the Controller itself? I see that I can use fillInActions which is simpler. But still seems action-oriented...
There was a problem hiding this comment.
@roblourens yeah, you have to use the controller since you are a tree. However once you tackle the debt item to convert aciton to commands this might simpllify itselft.
| @@ -139,6 +148,15 @@ KeybindingsRegistry.registerCommandAndKeybindingRule({ | |||
| } | |||
| }); | |||
|
|
|||
| MenuRegistry.appendMenuItem(MenuId.SearchContext, { | |||
| command: { | |||
| id: Constants.ReplaceActionId, | |||
There was a problem hiding this comment.
It is strange that this are called ActionId when you are actually registering a command
There was a problem hiding this comment.
Yes... will clean this up with the above debt item.
Agreed
Fixed
Will take this as a feature request, I can imagine lots of useful actions. I don't know if excluding an individual file is a common usecase though, especially given the X button. |
|
How about a context menu item to "Move to panel"/"Move to sidebar" |
|
@roblourens context menu action to Move View To Panel / Move View to Sidebar is an awesome idea. I would put it at the very end after a separtor. I would call it Move View to clarify that the whole thing will get moved actually. |
|
@roblourens ok I think you have adressed / created issues for most of my comments so this looks good to me. Let me know if you want me to re-review or test it out. But imho the best would be to merge and create a test plan item |
roblourens
force-pushed
the
roblou/searchContextMenu
branch
from
615de4a
to
6294fa2
Compare
@isidorn Would appreciate a quick review of this. The tree controller is just copied from elsewhere.
For #45063