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

Add command Copy Path and bind handler for native VS Code command #7934

Merged
merged 1 commit into from
Jun 2, 2020

Conversation

vzhukovs
Copy link
Contributor

@vzhukovs vzhukovs commented Jun 1, 2020

What it does

This changes proposal provides ability to copy absolute path to the file. This can be reached by calling Copy Path item in context menu of explorer or opened editor. Also, this proposal provides additional command copyFilePath, which is bridge between Theia and VS Code extension [1].

[1] microsoft/vscode/src/vs/workbench/contrib/files/browser/fileCommands.ts#L247-L259

Use case 1.
Copy Path call from Project Explorer:
copyFilePath_usecase_1

Use case 2.
Copy Path call from Opened Editor:
copyFilePath_usecase_2

Use case 3.
Call VS Code's command copyFilePath from third-party plugin:
copyFilePath_usecase_3

Use case 4.
Call VS Code's command copyFilePath from third-party plugin when everything hidden:
copyFilePath_usecase_4

Needed for Didact extension, but temporary can't be reached in the last one because of dependent issue. So there is another way to check this PR.

Signed-off-by: Vladyslav Zhukovskyi vzhukovs@redhat.com

How to test

Plugin to check the changes can be reached here.
For the calling items from context menus, it's enough to build Theia from the PR and try to copy file path.

Review checklist

  • Need to check on windows hotkey combination shift+alt+c, because of lack windows machine I can't be 100% sure, that this hotkey is free.

  • as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

@vzhukovs vzhukovs added enhancement issues that are enhancements to current functionality - nice to haves Team: Che-Editors issues regarding the che-editors team vscode issues related to VSCode compatibility core issues related to the core of the application labels Jun 1, 2020
@vzhukovs vzhukovs self-assigned this Jun 1, 2020
@azatsarynnyy
Copy link
Member

I've checked all the described use cases. Works well.
Thanks for providing the test plugin!

Signed-off-by: Vladyslav Zhukovskyi <vzhukovs@redhat.com>
@kittaakos
Copy link
Contributor

kittaakos commented Jun 8, 2020

Use case 2.
Copy Path call from Opened Editor:

@vzhukovskii, VS Code does not provide a dedicated context-menu item for copying the file path, did you add it intentionally? Can we get rid of it? It would be great to keep the context menu aligned with VS Code and not to end up in a gigantic one with all actions just because we can add it there. What do you think? Also, the keybinding you have defined has a collision with Cmd+Alt+C Match Case in the find widget.

@kittaakos
Copy link
Contributor

Since this change does not work on macOS (due to the Match Case of the Find/Replace widget), I tend to revert it. It would be great if you could prepare a follow-up PR without the context menu addition for the editor. Thoughts?

See the screencast:

  • I put something to the clipboard before executing Copy Path,
  • I still have my input on the clipboard instead of the path after executing Copy Path.

screencast 2020-06-08 14-48-36

@kittaakos
Copy link
Contributor

without the context menu addition for the editor.

We do not need the keybinding either.

@kittaakos
Copy link
Contributor

@vzhukovskii, @azatsarynnyy ping?

[1] microsoft/vscode/src/vs/workbench/contrib/files/browser/fileCommands.ts#L247-L259

It seems the VS Code implementation has a when condition for the keybinding, this PR does not define any when for the keybinding.

@akosyakov
Copy link
Member

@vzhukovskii Could you follow up on @kittaakos feedback please? 🙏

@kittaakos
Copy link
Contributor

Guys, I am going to wait till next week and will create a revert commit afterward.

@vzhukovs
Copy link
Contributor Author

vzhukovs commented Jun 11, 2020

@kittaakos, @akosyakov sorry for the late response, wasn't able to respond these days.

VS Code does not provide a dedicated context-menu item for copying the file path, did you add it intentionally?

Yes, it was added intentionally. We can move this item menu from editor context menu to the editor's tab context menu. As in VS Code, there is such item.
Снимок экрана 2020-06-11 в 12 46 55

Agree, from the editor context menu it should be moved away.

We do not need the keybinding either.

I can remove keybinding at all, is this sounds good? I think, keybinding should work, when !editorFocus will be added.

@kittaakos
Copy link
Contributor

As in VS Code, there is such item.

I see, so they put it there. Yes, it would be nice to do what VS Code does.

I can remove keybinding at all, is this sounds good?

Sounds good to me, but as you wrote adding the proper when would be the best, so we would be 100% VS Code compliant 💪 No hurry on the fixes, just let's not forget about this inconsistency. Thank you!

@vzhukovs
Copy link
Contributor Author

I see, so they put it there. Yes, it would be nice to do what VS Code does.

Good, I'll prepare a PR to move item from the current place to editor's tab.

but as you wrote adding the proper when would be the best, so we would be 100% VS Code compliant

This need to be checked and if there isn't any odd behavior, I'll include when condition to it. I didn't check the behavior of the keybinding inside opened editor, only when focus was in navigator and when there wasn't any part opened. My fault.

@kittaakos
Copy link
Contributor

Good, I'll prepare a PR

🙏 Thank you! Feel free to name me if you need someone for the review.

only when focus was in navigator and when there wasn't any part opened. My fault.

No worries at all; it happens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core issues related to the core of the application enhancement issues that are enhancements to current functionality - nice to haves Team: Che-Editors issues regarding the che-editors team vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants