Escaped strings (URIs) shown to users

[jonahgraham]

I think this is related to #358 / #368 but in the other direction.

If I have a URI comes back escaped e.g. "targetUri":"file:///tmp/www/data/simple/fi%C3%A9le.ts" then in the UI it sometimes displays the escaped values and sometimes the not escaped values.

(Originally observed while working on clangd, but I try to provide the similar thing for typescript as that is more accessible, the screenshots are with WildWebDev 1.0.1)

e.g. for two typescript files (with the é in the file name):

// file.ts
import { MyName } from './fiéle';

const x : MyName = {};
console.log(x);
// fiéle.ts
export interface MyName {};

Then the hover click shows the %C3%A9:

In the typescript case once the fiéle.ts file is open the hover click becomes correct:

[jonahgraham]

In typescript, the reason the display is escaped sometimes and not others appears to be what is coming from the server itself. "uri":"file:///tmp/www/data/simple/fi%C3%A9le.ts" vs "uri":"file:///tmp/www/data/simple/fiéle.ts"

logs of both cases

Without fiéle open:

{"jsonrpc":"2.0","id":"124","method":"textDocument/definition","params":{"textDocument":{"uri":"file:///tmp/www/data/simple/hello.ts"},"position":{"line":0,"character":25}}}

{"jsonrpc":"2.0","id":"125","method":"textDocument/typeDefinition","params":{"textDocument":{"uri":"file:///tmp/www/data/simple/hello.ts"},"position":{"line":0,"character":25}}}

{"jsonrpc":"2.0","id":"124","result":[{"originSelectionRange":{"start":{"line":0,"character":23},"end":{"line":0,"character":32}},"targetRange":{"start":{"line":0,"character":0},"end":{"line":0,"character":0}},"targetUri":"file:///tmp/www/data/simple/fi%C3%A9le.ts","targetSelectionRange":{"start":{"line":0,"character":0},"end":{"line":0,"character":0}}}]}

{"jsonrpc":"2.0","id":"125","result":[{"uri":"file:///tmp/www/data/simple/fi%C3%A9le.ts","range":{"start":{"line":0,"character":0},"end":{"line":0,"character":27}}}]}

With fiéle open:

{"jsonrpc":"2.0","id":"122","method":"textDocument/typeDefinition","params":{"textDocument":{"uri":"file:///tmp/www/data/simple/hello.ts"},"position":{"line":0,"character":26}}}

{"jsonrpc":"2.0","id":"123","method":"textDocument/definition","params":{"textDocument":{"uri":"file:///tmp/www/data/simple/hello.ts"},"position":{"line":0,"character":26}}}

{"jsonrpc":"2.0","id":"122","result":[{"uri":"file:///tmp/www/data/simple/fiéle.ts","range":{"start":{"line":0,"character":0},"end":{"line":0,"character":27}}}]}

{"jsonrpc":"2.0","id":"123","result":[{"originSelectionRange":{"start":{"line":0,"character":23},"end":{"line":0,"character":32}},"targetRange":{"start":{"line":0,"character":0},"end":{"line":0,"character":0}},"targetUri":"file:///tmp/www/data/simple/fiéle.ts","targetSelectionRange":{"start":{"line":0,"character":0},"end":{"line":0,"character":0}}}]}

[jonahgraham]

I can fix this very locally to where the hover is created, but I am not sure that is the correct place and I am wondering if the string uri should be converted to a java.net.URI much sooner.

Thoughts?

diff --git a/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/declaration/LSBasedHyperlink.java b/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/declaration/LSBasedHyperlink.java
index 4dd69d6..8654f42 100644
--- a/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/declaration/LSBasedHyperlink.java
+++ b/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/declaration/LSBasedHyperlink.java
@@ -14,6 +14,9 @@
  *******************************************************************************/
 package org.eclipse.lsp4e.operations.declaration;
 
+import java.net.URI;
+import java.net.URISyntaxException;
+
 import org.eclipse.jface.text.IRegion;
 import org.eclipse.jface.text.hyperlink.IHyperlink;
 import org.eclipse.lsp4e.LSPEclipseUtils;
@@ -117,7 +120,12 @@
 	}
 
 	private String getFileBasedLabel(String uri) {
-		return Messages.hyperlinkLabel + " - " + uri.substring(LSPEclipseUtils.FILE_URI.length()); //$NON-NLS-1$
+		try {
+			uri = new URI(uri).getPath();
+		} catch (URISyntaxException e) {
+			uri = uri.substring(LSPEclipseUtils.FILE_URI.length());
+		}
+		return Messages.hyperlinkLabel + " - " + uri; //$NON-NLS-1$
 	}
 
 }

[rubenporras]

Hi @jonahgraham , I think we should do the decoding on line 81 already, as we should decode all URIs, not only file URIs.

String uri = this.location.isLeft() ? this.location.getLeft().getUri() : this.location.getRight().getTargetUri()

Probably using URLDecoder.decode is good enough.

[mickaelistria]

I am wondering if the string uri should be converted to a java.net.URI much sooner.

In general, yes; the sooner we set the proper type, the less room we allow to errors. A PR that fixes it as suggested would probably be welcome.

[sebthom]

@jonahgraham is this issue still present? I cannot reproduce it. I get:

[jonahgraham]

I still see something similar when using clangd, I wonder if typescript server changed how it is encoding the data (see #474 (comment)). Perhaps I need to create a reproducible case in clangd (which is getting easier to do as cdt-lsp is part of SimRel these days).

I don't know where in the stack the two entries are being populated from, but the commands I see are textDocument/declaration and textDocument/documentLink, both returning uris of the form "file:///usr/include/c%2B%2B/12/iostream"