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.

Sign up for GitHub

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

Jump to bottom

Copy Path and Copy Relative Path commands are only available when editor is not focused. #137216

Closed
a-stewart opened this issue Nov 15, 2021 · 8 comments · May be fixed by #137217
Closed

Copy Path and Copy Relative Path commands are only available when editor is not focused. #137216

a-stewart opened this issue Nov 15, 2021 · 8 comments · May be fixed by #137217
Assignees
@bpasero
Labels
bug Issue identified by VS Code Team member as probable bug verified Verification succeeded workbench-tabs VS Code editor tab issues
Milestone
August 2022

Comments

@a-stewart
Copy link
Contributor

a-stewart commented Nov 15, 2021 •
edited

Does this issue occur when all extensions are disabled?: Yes

  • VS Code Version: 1.62.1
  • OS Version: Mac OS 11.6

Steps to Reproduce:

  1. Open a file
  2. Right click on the file tab, there is a copy file command and a keybinding to use it
  3. Try and use the suggested keybinding (cmd + alt + c) (whilst the editor is focused)
  4. Nothing is added to the clipboard and nothing is shown to the user explaning why.

I would expect the path to be copied to clipboard but it isn't.

The when clauses in this file, specify that the keybinding is only active when the editor is not focused.
https://github.com/microsoft/vscode/blob/main/src/vs/workbench/contrib/files/browser/fileCommands.ts#L293-L319

I did some digging to see when that when clause was added and found this:

Related change: c88e58f
Which fixed bug: #47947

The find widget has 3 toggles:

  • match case - cmd+alt+C
  • match whole word - cmd+alt+W
  • use regular expression - cmd+alt+R

From those 3 there are 2 commands which now overwrite :

  • revealFileInOS steals cmd+alt+R
  • copyFilePath steals cmd+alt+C

@isidorn Please leave cmd+alt+R and cmd+alt+C for the find widget toggles when focus is in the editor.

Originally posted by @alexdima in #47947 (comment)

It looks like the exclusion is correct, but it is a bit confusing for a user as when you right click on a tab, you are shown a command to copy the relative path, but then when you try and use it, it doesn't work. (Editor being focused is probably the most common use case in vscode.)

As an alternative suggestion, perhaps we could use !findWidgetVisible as the when clause instead - that would avoid conflicting with the find widget commands but would allow the command to be used in the editor.

It also has the advantage that something would always happen, either the path would be copied, or the icon in the find widget would change.

I have created a draft CL with a change that would potentially improve this.

WDYT?
@a-stewart a-stewart mentioned this issue Nov 15, 2021
Open
@alexdima alexdima assigned bpasero and unassigned alexdima Nov 18, 2021
@bpasero bpasero added this to the November 2021 milestone Nov 18, 2021
@bpasero bpasero added the help wanted Issues identified as good community contribution opportunities label Nov 18, 2021
@bpasero bpasero mentioned this issue Nov 24, 2021
Closed
@bpasero bpasero assigned rebornix and unassigned bpasero Nov 24, 2021
@bpasero
Copy link
Member

bpasero commented Nov 24, 2021 •
edited

Related PR: #137217

@rebornix assigning this issue also to you given the change in #49231. I think the check for EditorContextKeys.focus is too strict now (hence this issue). The PR seems to solve #49231 slightly better by explicitly checking for the find input box having focus, but to be honest I find such hardcoding from the explorer world weird. I would have somehow expected the find widget keybinding to have higher priority than the one from the workbench, but maybe that is not the case.

//cc @JacksonKearl

@bpasero bpasero removed the help wanted Issues identified as good community contribution opportunities label Nov 24, 2021
@bpasero bpasero removed this from the November 2021 milestone Nov 24, 2021
@bpasero bpasero added the file-explorer Explorer widget issues label Nov 24, 2021
@a-stewart
Copy link
Contributor Author

@rebornix - was just wondering if you had any opinions on this - I'm happy to look into alternatives if you have a preferred alternative approach?

@bpasero bpasero mentioned this issue Dec 14, 2021
Closed
@a-stewart
Copy link
Contributor Author

a-stewart commented Feb 8, 2022 •
edited

@rebornix @bpasero - Was just wondering if there was any update on this? PR #137217 is ready for reviewing if tightening the when clause is the approach you would like to take. Otherwise happy to work on another option.

@a-stewart
Copy link
Contributor Author

@rebornix @bpasero - small bump - it's not a major issue but was wondering if you had any thoughts about this?

@rebornix rebornix added this to the July 2022 milestone Jul 26, 2022
@rebornix
Copy link
Member

I'll take a look this week and see the impact of merging the PR and decide if having it merged this week or next.

@rebornix rebornix modified the milestones: July 2022, August 2022 Jul 27, 2022
@rebornix
Copy link
Member

rebornix commented Aug 5, 2022

Experimented with the PR and a couple of variations of when clauses, but found we could'n't really get rid of editorFocus. Toggle Match Case is not only used in Find Widget, but that also drives how cmd+d works. So users should be able to press cmd+alt+c in the editor (without Find Widget being visible) and then select words that match the current selection.

It looks like the exclusion is correct, but it is a bit confusing for a user as when you right click on a tab, you are shown a command to copy the relative path, but then when you try and use it, it doesn't work

This is a different/bigger issue though for Menu. Our keybindings on menu/toolbars don't present when clause, there is no guarantee that pressing the keybinding from the menu anywhere can work.

@rebornix rebornix added the under-discussion Issue is under discussion for relevance, priority, approach label Aug 5, 2022
@rebornix rebornix modified the milestones: August 2022, On Deck Aug 5, 2022
@bpasero bpasero modified the milestones: On Deck, August 2022 Aug 6, 2022
@bpasero
Copy link
Member

bpasero commented Aug 6, 2022

@alexdima fyi this is about the fact that a context menus shows a keybinding even though the context does not match. I am pinging you because I see similar issue #40389 which was closed.

The commands in question are the "Copy" commands in the tab context menu:

image

The command is registered with a when clause here:

vscode/src/vs/workbench/contrib/files/browser/fileCommands.ts

Lines 266 to 278 in cb79d01

weight: KeybindingWeight.WorkbenchContrib,
when: EditorContextKeys.focus.toNegated(),
primary: KeyMod.CtrlCmd | KeyMod.Shift | KeyMod.Alt | KeyCode.KeyC,
win: {
primary: KeyChord(KeyMod.CtrlCmd | KeyCode.KeyK, KeyMod.CtrlCmd | KeyMod.Shift | KeyCode.KeyC)
},
id: COPY_RELATIVE_PATH_COMMAND_ID,
handler: async (accessor, resource: URI | object) => {
const resources = getMultiSelectedResources(resource, accessor.get(IListService), accessor.get(IEditorService), accessor.get(IExplorerService));
await resourcesToClipboard(resources, true, accessor.get(IClipboardService), accessor.get(ILabelService), accessor.get(IConfigurationService));
}
});

And the menu contributed here:

vscode/src/vs/workbench/contrib/files/browser/fileActions.contribution.ts

Lines 154 to 168 in cb79d01

// Editor Title Context Menu
appendEditorTitleContextMenuItem(COPY_PATH_COMMAND_ID, copyPathCommand.title, ResourceContextKey.IsFileSystemResource, '1_cutcopypaste');
appendEditorTitleContextMenuItem(COPY_RELATIVE_PATH_COMMAND_ID, copyRelativePathCommand.title, ResourceContextKey.IsFileSystemResource, '1_cutcopypaste');
appendEditorTitleContextMenuItem(REVEAL_IN_EXPLORER_COMMAND_ID, nls.localize('revealInSideBar', "Reveal in Explorer View"), ResourceContextKey.IsFileSystemResource, '2_files', 1);
export function appendEditorTitleContextMenuItem(id: string, title: string, when: ContextKeyExpression | undefined, group: string, order?: number): void {
// Menu
MenuRegistry.appendMenuItem(MenuId.EditorTitleContext, {
command: { id, title },
when,
group,
order
});
}

My question is whether there is something the context menu does wrong or whether we simply do not support when in context menus.

I think bottom line, the desired output would be to:

  • show the command in the context menu
  • do not show the keybinding unless the when condition matches

@rebornix rebornix removed their assignment Aug 8, 2022
bpasero added a commit that referenced this issue Aug 19, 2022
@bpasero
Copy Path and Copy Relative Path commands are only available when edi…
2facb3a 
…tor is not focused. (fix #137216)
@bpasero bpasero added bug Issue identified by VS Code Team member as probable bug and removed under-discussion Issue is under discussion for relevance, priority, approach labels Aug 19, 2022
@bpasero
Copy link
Member

bpasero commented Aug 19, 2022

This was fun to investigate, learnings:

  • if a command only has 1 keybinding assigned, we always show that in the menus even if context keys do not match as a way to let the user learn a possible keybinding (explainer)
  • this means we have to register another rule for when editor does have focus to properly show it in the context menu

I decided to go with a chord in that case:

image

As a nice side effect, you can now copy path and copy relative path from within the editor without having to rebind keybindings.

@bpasero bpasero added workbench-tabs VS Code editor tab issues and removed file-explorer Explorer widget issues labels Aug 19, 2022
@bpasero bpasero closed this as completed Aug 19, 2022
GulajavaMinistudio pushed a commit to GulajavaMinistudio/vscode-1 that referenced this issue Aug 19, 2022
@bpasero
Copy Path and Copy Relative Path commands are only available when edi…
ff8d0d6 
…tor is not focused. (fix microsoft#137216) (microsoft#158556)
@DonJayamanne DonJayamanne added the verified Verification succeeded label Aug 24, 2022
@bpasero bpasero mentioned this issue Sep 7, 2022
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@bpasero bpasero

Labels
bug Issue identified by VS Code Team member as probable bug verified Verification succeeded workbench-tabs VS Code editor tab issues
Projects
None yet
Milestone
August 2022
5 participants
@a-stewart @rebornix @bpasero @DonJayamanne @alexdima