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

fix #5917: normalize the path of terminal link before openning it #5918

Merged
merged 1 commit into from
Aug 14, 2019

Conversation

a1994846931931
Copy link
Contributor

@a1994846931931 a1994846931931 commented Aug 12, 2019

Signed-off-by: Cai Xuye [email protected]

What it does

Normalize the paths of file links contributed by terminals to avoid opening multiple editors for the same file.

How to test

  1. Create a new file, e.g. named package.json and open it
  2. Print some file links in the terminal, for example:
  • run echo "./package.json"
  • run echo ".///package.json"
  • run echo "../${parent}/package.json"
  • run echo "/${absolute/path/to/package.json}"
  1. Ctrl/cmd+click open these links and see if they all activate the same editor opened in step 1

Review checklist

Reminder for reviewers

@a1994846931931
Copy link
Contributor Author

I currently fix it in the terminal-linkmatcher-files level. But we can also do it in EditorManager.open and convert all URI parameters to normalized ones. Which one is better? Is there a situation where these URIs that actually refer to the same file SHOULD be regarded as different ones?

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normalization should happen here: https://github.com/theia-ide/theia/blob/d9bb466013049e272666bdbe3aee60d74ecd30f3/packages/core/src/browser/navigatable.ts#L89-L91

that it fixes this issue for all navigatable open handler (editors, previews, and so on)

@akosyakov
Copy link
Member

akosyakov commented Aug 13, 2019

We cannot use Node.js apis, i.e. path module, in browser module. We either have to call to the server for normalization or our Path should implement normalize method, maybe using some existing npm package. Although i am not sure whether normalize method should be aware about backend operating system, if so then it can be done only by backend, i.e. paths should not be normalized against frontend OS.

@akosyakov akosyakov added terminal issues related to the terminal shell issues related to the core shell labels Aug 13, 2019
@a1994846931931
Copy link
Contributor Author

We cannot use Node.js apis, i.e. path module, in browser module. We either have to call to the server for normalization or our Path should implement normalize method, maybe using some existing npm package. Although i am not sure whether normalize method should be aware about backend operating system, if so then it can be done only by backend, i.e. paths should not be normalized against frontend OS.

@akosyakov Code updated. Changes include:

  1. Normalize uri for all NavigatableWidgetOpenHandler (instead of normalization in EditorManager or link-matcher)
  2. Path.normalize is implemented. Corresponding tests are added to path.spec.ts as well.

@a1994846931931
Copy link
Contributor Author

Theia's Path supports both POSIX and Windows, but I have only tested it on Ubuntu. Could anyone try to test it on other platforms? @akosyakov

@akosyakov
Copy link
Member

akosyakov commented Aug 14, 2019

@a1994846931931 cool! actually i don't think we need to care about POSIX and Windows, our URI always normalize path to use / as a separator, so it should not be platform dependent

I'm testing it now.

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works very well. I like API improvements as well.

@akosyakov
Copy link
Member

akosyakov commented Aug 14, 2019

@a1994846931931 i will invite you to Theia org as a contributor and after merging your PRs nominate as Theia committer. Quality of your PRs is superb!

@a1994846931931
Copy link
Contributor Author

a1994846931931 commented Aug 14, 2019

@a1994846931931 i will invite you to Theia org as a contributor and after merging your PRs nominate as Theia committer. Quality of your PRs is superb!

@akosyakov Hey, thanks! It's a great honor!

And the code is updated now.

…alizePath method; 3. normalize uri for NavigatableWidgetOpenHandler

Signed-off-by: Cai Xuye <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
shell issues related to the core shell terminal issues related to the terminal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants