Normalize URI's before interacting with recentworkspaces.json

[colin-grant-work]

What it does

Fixes #11013 by normalizing URI's before comparing them with the contents of the recentworkspaces.json file.

How to test

1. Open a few workspaces
2. Use the Open recent workspace command
3. Observe that the current workspace is not listed twice

Review checklist

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

Reminder for reviewers

• As a reviewer, I agree to behave in accordance with the review guidelines

[kittaakos]

Hi, can you explain what this change does? I wonder why you name the variable uri when it's a path. Any concerns about backward compatibility?

[colin-grant-work]

The current code treats recentworkspaces.json as a record of unique past workspaces, so it needs to be sure that the strings it's dealing with are consistently formatted. In principle, it would be fine if they were all full stringified URI's - then the file is less human readable, but that isn't really its purpose - if you strongly prefer that solution. The hitch there is that we can't guarantee that the caller has sent the the scheme, so we'd have to be OK with a default of file. Re: the name uri, that was mostly to limit the diff (the subsequent code after the normalization doesn't have to be changed), but you're right, path would be more accurate.

[kittaakos]

The hitch there is that we can't guarantee that the caller has sent the the scheme

I am not sure if Theia's default workspace server impl has to be fault-tolerant in this case. The code you have changed worked fine in the last four years or so.

When does the FE send a URI without the scheme? Did you manage to figure it out? Maybe one or two tests could easily show that you're right, and that's the fix. But I cannot agree on the commit message, title, and PR description:

by normalizing URI's

You're not normalizing the URI, but changing the URI to an FS path.

If the recentworkspaces.json file always stored URIs, it would be great not to switch to FS paths. No changelog, no tests, nothing just three steps I can do to verify the UI.

After checking out your branch and trying it on Windows, this is the recentworkspaces.json file content on my disk:

{"recentRoots":["\\","\\Users\\kittaakos\\dev\\git\\theia\\doc"]}

If I open a folder as a workspace, then the rawUri is file:///c%3A/Users/kittaakos/scoop, the FS path that you named uri is c:\\Users\\kittaakos\\scoop. We set this as the root URI, and nothing happens. Theia opens without a workspace, or open on my C: root. Open recent workspaces show this:

I propose revisiting this PR. I can help with the verification on Windows. What do you think?

[colin-grant-work]

I am not sure if Theia's default workspace server impl has to be fault-tolerant in this case.

The trouble is that the framework failed to implement a good serialization for URI's, and so there are a lot of signatures for exchanges between the backend and frontend like the one I'm changing:

                uri: string
                ~~~  ******
What we hope to get  What we can be (fairly) sure we'll get

Because we can't guarantee that we'll get correctly formatted full URI's, odd things can happen. #11013 is one such, where a mix of stringified URI's and stringified paths lets a duplicate slip through. It looks like the code I've written causes more problems, as per @marol1210's comment, and I'm grateful for your checks on Windows. However, that seems like a problem with the FileUri.fsPath implementation: if garbage is being generated, then it must be there, since it's the only thing touching the incoming data, where it should be able to consistently format intelligible paths on all of our supported OS's. We can dodge that problem (though it should be fixed), by using URI.toString(), and I'm happy to do that, but I would still say that the backend workspace server should take responsibility for normalizing what it receives to avoid problems like #11013. What do you think?

[kittaakos]

we can't guarantee that we'll get correctly formatted full URI's, odd things can happen

Why not try to parse the string URI to a URI object? If it fails, it fails; the FE has sent garbage.

I do not understand why you want to use FS paths as the workspace root URI. I might not be aware of some recent changes in Theia; please help me. Let's assume the following happy path: this method receives a full valid URI on Windows as a string: 'file:///c%3A/path/to/resource', then you convert it to FS path string: 'c:\\path\\to\\resource', and we set this string as the workspace URI.

I think I get what you want to achieve; the file:///path/to/resource URI on UNIX is the same as the FS path if you drop the scheme: /path/to/resource, but it won't work on Windows. Maybe you could try to convert back to a URI and see if it fixes your problem. (I think VS Code can parse URI without a scheme, but then it will be converted to file. Please check if the back conversion solves your issue.) Something like this:

    async setMostRecentlyUsedWorkspace(rawUri: string): Promise<void> {
        const uri = FileUri.create(FileUri.fsPath(rawUri)).toString(); // <---- (I edited this line and added the missing `.toString()`, but preferably the deferred `root` should have a `URI | undefined` type for safety, just as you have mentioned.)
        this.root = new Deferred();
        this.root.resolve(uri);
        // and the rest of the method
    }

To sum up, I think we cannot switch from URI to FS path like this. This would be a breaking change for all Windows users. Thoughts? Thanks!

[colin-grant-work]

Why not try to parse the string URI to a URI object? If it fails, it fails; the FE has sent garbage.

As I've said, I am happy to use URI's:

...it would be fine if they were all full stringified URI's...
...by using URI.toString()...

I'll make the changes.

[marol1210]

in window OS , recentworkspace.json file is malformed. redundant-workspace , too many here .

[msujew]

@colin-grant-work Some tests for the recentRoots property fail now, since they expect file paths, not URIs.

Otherwise I can confirm that this change addresses the issue of duplicate Open Recent Workspace entries.

On windows, the entries are incorrectly formatted, but that's related to another issue, see #10591.

[colin-grant-work]

@msujew, what will be required to resolve the issues with URI's on Windows? Will your other PR fix it automatically, or should the frontend be using your new Path.fsPath utility?

[msujew]

@colin-grant-work Yep, they will need to use the Path.fsPath method. I created an issue to track any code that needs this in the future here: #10830. Feel free to add any entries you find.

[msujew]

Changes are looking good 👍 I can confirm that the most recent workspace is no longer displayed twice.