fernandoescolar / vscode-solution-explorer

This is a Visual Studio Code extension that provides a (.sln) Visual Studio Solution explorer panel..
MIT License
346 stars 73 forks source link

Fixed issue #5 - keyboard shortcuts not working. #253

Closed panoskj closed 1 year ago

panoskj commented 1 year ago

There were three problems:

  1. For some reason, key bindings need to check for the "focusedView" instead of the "view" in their "when" clauses.
  2. The check for "viewItem" has to be completely removed from "when" clauses.
  3. Command handlers aren't given a TreeItem when the command runs form a keybinding. An alternative way to get the current selection is needed.

https://github.com/microsoft/vscode/issues/72442 confirms what I found out. It looks like VSCode doesn't associate key press events with the selected tree items currently. That's why the context menu works but shortcuts don't.

The solution is to get the selected TreeItem directly from the treeview. This has the advantage that you can also get all selected items if needed (e.g. the Delete command should delete all selected items instead of one). As for checking the view item type, this can be done inside the command handler's code. I opted for passing the required view item type as an argument of the key binding's command to minimize changes in the code, but it could have been hardcoded in each command instead.

Since the "when" clauses became the same for some commands after removing "viewItem", only the last registered command would be executed. So the next step was to combine all commands into one for each key. I added a helper command that accepts a list of commands and their arguments to do this, in order to minimize changes in the code.

In addition, this PR updates the "Delete" command to run for all selected files instead of the last selected file. As a result of the implementation, when deleting multiple files the user will be asked whether they are sure they want to delete each file separately. To address this problem, a new delete command that unifies existing delete commands can be added.

Another side effect of this PR is that the key bindings will not be visible in the context menu (since they don't run the same command anymore). It looks like this problem can be addressed if all commands are "unified". For example, currently there are 3 different types of rename commands.

In conclusion this PR will work, but I feel it isn't complete. So let me know whether you would like me to add the "unified" commands I suggested. In which case, would you prefer to keep existing commands (and end up with 4 rename commands for example) or simply merging them into one? Do you prefer the required view item of each command to be hardcoded or to keep it in package.json?

I just want some feedback from you before I try to refactor more code.

fernandoescolar commented 1 year ago

This problem has been there since day one and I think you have found a very ingenious solution. Thank you very much.

Just a couple of notes:

I have been thinking for a long time how to improve the commands issue, not to have a package.json file so long and to be able to control them from typescript code, although I have not found yet a good way to do it.

panoskj commented 1 year ago

I would move to a file named src/commands/KeyBindingCommand.ts the logic added to the src/SolutionExplorerCommands.ts file. This way we would have isolated and controlled the operation of these special commands.

You are right, so I will add this change, unless...

To continue having the key bindingns in the context menus, I would keep the old settings which are harmless. Although we could refactor this part.

Refactoring to merge/unify commands as I suggested would both make package.json smaller and KeyBindingCommand redundant, while allowing a better implementation for delete (multiple files). The idea is that commands depend on the current selection anyway, so there is no point in allowing the user to execute each command separately, e.g. to run the "rename file" command when a folder is the current selection. So if we go for a refactoring, the "underlying" commands can be completely removed from JSON and hidden from user (but this is not necessary). If you agree, I can do this refactoring and get rid of KeyBindingCommand.

Edit: after thinking more about it, I will add a unified delete command that handles multiple files properly and then you can decide if you would like to do this refactoring for all commands.

Thanks for merging the other two PRs so quickly by the way.