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

workbench.action.files.copyPathOfActiveFile is missing in 1.20 #41502

Closed
Tyriar opened this issue Jan 12, 2018 · 23 comments
Closed

workbench.action.files.copyPathOfActiveFile is missing in 1.20 #41502

Tyriar opened this issue Jan 12, 2018 · 23 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug candidate Issue identified as probable candidate for fixing in the next release file-explorer Explorer widget issues verified Verification succeeded
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Jan 12, 2018

It's in 1.19 but is gone in 1.20, this is the old keybinding:

{ "key": "cmd+k p",               "command": "workbench.action.files.copyPathOfActiveFile" },

If the command was replaced with something we should add this keybinding back.

@Tyriar Tyriar added the bug Issue identified by VS Code Team member as probable bug label Jan 12, 2018
@bpasero bpasero assigned isidorn and unassigned bpasero Jan 12, 2018
@bpasero
Copy link
Member

bpasero commented Jan 12, 2018

@isidorn maybe a regression from your commands work?

@isidorn isidorn added this to the January 2018 milestone Jan 12, 2018
@isidorn isidorn added the file-explorer Explorer widget issues label Jan 12, 2018
isidorn added a commit to microsoft/vscode-docs that referenced this issue Jan 12, 2018
@isidorn
Copy link
Contributor

isidorn commented Jan 12, 2018

Same as workbench.action.files.revealActiveFileInWindows
The workbench.action.files.copyPathOfActiveFile is a duplication of copyFilePath command. I will continue supporting workbench.action.files.copyPathOfActiveFile for January and will drop it in february. I have documented this in the release notes.

@Tyriar thanks for finding this. I will now go through other commands to verify I did not remove more accidentely

@isidorn
Copy link
Contributor

isidorn commented Jan 12, 2018

Ok here are more things which I accidentely removed
workbench.action.files.newFile
workbench.action.files.newFolder
workbench.action.files.saveFiles -> duplication of workbench.action.files.saveAllFiles This one I will also completely drop in february

Edit: actually the first two have been deprecated by ben already in the past, now I just killed them for good. Will still update the docs for the saveFiles one.

Edit2: @bpasero maybe I leave both save commands since maybe a lot of users use them. If we keep one which one to keep. What do you think?

Edit3: will keep both saveFiles and saveAllFiles

isidorn added a commit that referenced this issue Jan 12, 2018
isidorn added a commit that referenced this issue Jan 12, 2018
@isidorn
Copy link
Contributor

isidorn commented Jan 12, 2018

@bpasero I have now transitioned the newly introduced ids to be short and elegant. The last commit captures that. All of these ids were not present in vscode before so we can name them as we want

@bpasero
Copy link
Member

bpasero commented Jan 12, 2018

@isidorn I am not seeing any command with the ID workbench.action.files.saveAllFiles?

@bpasero
Copy link
Member

bpasero commented Jan 12, 2018

@isidorn the original intent as far as I remember was to have an action that would save all files, but not untitled. As such, the save all action is not a replacement to that as it would popup a dialog asking where to save untitled files. So we should still have 2 actions, one for saving all (untitled and files) and one for files only.

@isidorn
Copy link
Contributor

isidorn commented Jan 12, 2018

@bpasero all is good know. We will have those two different
saveAll new one (previously was not exposed)
and workbench.action.files.saveFiles the old one which I brought back.

@bpasero
Copy link
Member

bpasero commented Jan 12, 2018

Oke

@Tyriar
Copy link
Member Author

Tyriar commented Jan 12, 2018

I have now transitioned the newly introduced ids to be short and elegant.

Is it just me that preferred the longer command names and found the short ones to be messy? Also Make sure that the keybindings are moved over to whichever one we're keeping as I rely on the defaults for that (so I'm sure others would).

@isidorn
Copy link
Contributor

isidorn commented Jan 15, 2018

@Tyriar we are not changing any command names, just the newly introduced ones will be short.

@Tyriar
Copy link
Member Author

Tyriar commented Jan 18, 2018

just the newly introduced ones will be short

@isidorn the long namespaced commands are good though, if we don't use namespaces going forward it's going the list of commands is going to be a mess.

@isidorn
Copy link
Contributor

isidorn commented Jan 19, 2018

@Tyriar we already have some in stable which are short. For me personally one namespace makes sense, 3 are just too much and most of them are bogus. Like why have workbench.action.files.copy when it should just be files.copy

Though it is up to each command author to choose what to do, currently we do not have a vscode wide convention

@octref octref added the verified Verification succeeded label Feb 1, 2018
@octref
Copy link
Contributor

octref commented Feb 1, 2018

@isidorn

The workbench.action.files.copyPathOfActiveFile is a duplication of copyFilePath command. I will continue supporting workbench.action.files.copyPathOfActiveFile for January and will drop it in february.

They are not duplicates. The current copyPathOfActiveFile copies active editor's file path, whereas copyFilePath copies the active file on explorer.

In cases where explorer.autoReveal is false, or workbench.list.openMode is doubleClick, the active file on explorer can be different than the one open in active editor.

@isidorn
Copy link
Contributor

isidorn commented Feb 2, 2018

@octref you are correct. However I question how many users are relying on this behavior. I would bet not many, but I might be wrong.

@Tyriar
Copy link
Member Author

Tyriar commented Feb 2, 2018

@isidorn didn't @bpasero add a bunch of keybindings to help users navigating the explorer? I think the following wouldn't work anymore:

  • ctrl+shift+e
  • down x times to some file
  • copy file's path command

I think the only way to do this now is via the context menu key if it's available and selecting Copy Path.

@bpasero
Copy link
Member

bpasero commented Feb 3, 2018

@Tyriar @isidorn yeah makes sense, I think we need to revive that command to not cause a regression.

@Tyriar Tyriar reopened this Feb 4, 2018
@Tyriar Tyriar removed the verified Verification succeeded label Feb 4, 2018
@isidorn
Copy link
Contributor

isidorn commented Feb 5, 2018

@bpasero the command workbench.action.files.copyPathOfActiveFile does not need reviving, since I never removed it.
I just planned to remove it in Febraury but will not do that now.

Closing as this works nicely in insiders.
@bpasero @Tyriar please reopen if you see otherwise

@isidorn isidorn closed this as completed Feb 5, 2018
@isidorn
Copy link
Contributor

isidorn commented Feb 5, 2018

Ok the issue is that the command now first looks at the explorer and only then at the active file. So reopening to change that.
Not sure what would be the optimal behavior if there is no open file? No op then?

@isidorn isidorn reopened this Feb 5, 2018
@isidorn isidorn added the candidate Issue identified as probable candidate for fixing in the next release label Feb 5, 2018
@isidorn isidorn closed this as completed in 5b6a681 Feb 5, 2018
isidorn added a commit that referenced this issue Feb 5, 2018
@isidorn
Copy link
Contributor

isidorn commented Feb 5, 2018

I have brought back the old behavior.

  • workbench.action.files.copyPathOfActiveFile copies the path of the active file, if there is no editor active this command is a no op
  • copyFilePath looks at what is focused in the explorer / open editors as it did before

Apart from this I aslo noticed a simliar issue with workbench.action.files.revealActiveFileInWindows and revealFileInOS so I also tackled that in this commit.

  • revealFileInOS works on explorer / open editors specifc context
  • workbench.action.files.revealActiveFileInWindows looks at the active editor

@bpasero can you please review / verify once the new builds are out. I have cherry picked this on top of the release branch

@bpasero bpasero added the verified Verification succeeded label Feb 5, 2018
@bpasero
Copy link
Member

bpasero commented Feb 5, 2018

Verified and reviewed (out of sources). 👍

@Tyriar
Copy link
Member Author

Tyriar commented Feb 5, 2018

@isidorn the keybinding cmd/ctrl+k, p is no longer attached to the command. I would probably report an issue about this if I was a regular user and it stopped working in stable.

@Tyriar Tyriar reopened this Feb 5, 2018
@Tyriar Tyriar removed the verified Verification succeeded label Feb 5, 2018
@isidorn isidorn closed this as completed in d7f097c Feb 5, 2018
isidorn added a commit that referenced this issue Feb 5, 2018
@isidorn
Copy link
Contributor

isidorn commented Feb 5, 2018

@Tyriar that's a great catch.
I have brought back the missing keybindings and I have also cherry-picked this on top of the release branch.
I have also went through all the commands and made sure that none of the keybindings have been broken compared to stable.
Verifier: also verify that all default keybindings are still present. This is easiest to verify by having stable and insider running side by side and checking keyboard shorcuts searching for 'files.'

@ramya-rao-a ramya-rao-a added the verified Verification succeeded label Feb 6, 2018
@Tyriar
Copy link
Member Author

Tyriar commented Feb 6, 2018

Double verified, I have my keybinding back 😄 🎉

@vscodebot vscodebot bot locked and limited conversation to collaborators Mar 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug candidate Issue identified as probable candidate for fixing in the next release file-explorer Explorer widget issues verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

5 participants