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

Custom domain URLs in JSON include trailing quote #62800

Closed
pvh opened this issue Nov 8, 2018 · 10 comments
Closed

Custom domain URLs in JSON include trailing quote #62800

pvh opened this issue Nov 8, 2018 · 10 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug remote Remote system operations issues verified Verification succeeded
Milestone

Comments

@pvh
Copy link

pvh commented Nov 8, 2018

  • VSCode Version: VSCode Electron 3 exploration
  • OS Version: Windows 10

Steps to Reproduce:

  1. Register a filesystem with a new protocol
  2. Include a link to that filesystem in a JSON document (in my case this document shows the defect):
{ 
  "good-url": "http://doc2.json",
  "bad-url": "hypermergefs:/doc2.json",
  "unhighlighted-url": "unregistered:/foo.json"
}
  1. Note that the "bad-url" highlight includes the trailing quote and the unregistered scheme does not highlight.

I suspect this is a Very Small Bug with a regex somewhere but I haven't been able to find it. If anyone can point me in the right direction I'm happy to fix it.

@vscodebot vscodebot bot added the editor-contrib Editor collection of extras label Nov 8, 2018
@chrmarti chrmarti removed the editor-contrib Editor collection of extras label Nov 9, 2018
@alexr00
Copy link
Member

alexr00 commented Nov 9, 2018

@pvh, can you outline what you mean by "Register a filesystem with a new protocol"? What you have looks like a registered URI protocol but I can't repro with that.

@alexr00 alexr00 added the info-needed Issue requires more information from poster label Nov 9, 2018
@pvh
Copy link
Author

pvh commented Nov 9, 2018

Ah - yes, I found a reproduction with a publicly available filesystem provider. Try installing the SSH FS extension and then look at the above JSON with the extension loaded. You'll see that the SSH extension URL highlight includes both the trailing " and comma. Does that help?

@alexr00
Copy link
Member

alexr00 commented Nov 12, 2018

I still can't reproduce this on the insiders build or on stable. Does it reproduce on those for you? Can you add a screen shot of what you're seeing with SSH FS please?

@pvh
Copy link
Author

pvh commented Nov 12, 2018

Sorry, I should have been more clear that to see the bug with the SSH FS you need an ssh URL. On both the insiders and exploration build I have I see this:

image

Note the underline extends through the ", .

For reference, the Insiders build I'm using is the following:

Version: 1.29.0-insider (user setup)
Commit: 1d0e429
Date: 2018-11-06T10:07:08.614Z
Electron: 2.0.12
Chrome: 61.0.3163.100
Node.js: 8.9.3
V8: 6.1.534.41
Architecture: x64

@alexr00
Copy link
Member

alexr00 commented Nov 13, 2018

I can see it repro now. The step I used:

Install SSH FS
Add an arbirary sshfs.configs to settings. I just copied it out of the extension info.
Run the SSH FS: Connect as workspace folder command with your new dummy config
Type up this example:

{
    "good-url": "http://doc2.json",
    "bad-url": "ssh://microsoft.com/doc2.json",
    "unhighlighted-url": "unregistered://foo.json"
}

See that the underline for "bad-url" extends through the comma.

@jrieken, do you usually look at URI document link provider issues?

@alexr00 alexr00 removed the info-needed Issue requires more information from poster label Nov 13, 2018
@jrieken jrieken added bug Issue identified by VS Code Team member as probable bug remote Remote system operations issues labels Nov 13, 2018
@jrieken jrieken self-assigned this Nov 13, 2018
@jrieken
Copy link
Member

jrieken commented Nov 13, 2018

Yeah, we have simple (read stupid) link detector when a file system is being registered: https://github.com/Microsoft/vscode/blob/95190ed713a1c0312686a04db5ce8a25db866c27/src/vs/workbench/api/node/extHostFileSystem.ts#L37-L39

@pvh
Copy link
Author

pvh commented Nov 14, 2018

Aha! Yeah, I suspect you could just throw some quotes into the [^\s] group at the end there but to do it right I guess you'd probably want to handle quote escaping and things like that. I took a quick look at other implementations of provideDocumentLinks elsewhere in the main repo and none of them popped out as having an obviously equivalent regex. Let me know if this is something you could use an external contribution for but I suspect it's probably a quick fix for someone who knows the code well.

@jrieken
Copy link
Member

jrieken commented Nov 15, 2018

I suspect you could just throw some quotes into the [^\s] group at the end there but to do it right I guess you'd probably

sounds like a winner ;-)

@jrieken jrieken added this to the November 2018 milestone Nov 15, 2018
jrieken added a commit that referenced this issue Nov 15, 2018
@jrieken
Copy link
Member

jrieken commented Nov 15, 2018

Nah, I did go for the proper fix... It now uses the state machine logic we have for http and file links

@pvh
Copy link
Author

pvh commented Nov 15, 2018

You, sir, are a gentleman and a scholar. Thanks!

@mjbvz mjbvz added the verified Verification succeeded label Dec 6, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators Dec 30, 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 remote Remote system operations issues verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

5 participants