workspace: normalize paths of workspace roots

[corneliusludmann]

What it does

This is my proposal to fix #7597 .

If you have configured a workspace folder like

"path": "../../workspace"

you get a Terminal for URI /workspace/theia/../../workspace instead of /workspace. This commit fixes this by using a normalized path in the terminal cwd selector.

An alternative would be to fix the roots in the workspace service directly so that it benefits everywhere.

How to test

see #7597

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

[vince-fugnitto]

@corneliusludmann thank you, I hadn't thought of the use case when I initially authored the changes. Would the suggestion you had of updating the WorkspaceService be better which would likely fix other similar issues?

[akosyakov]

@vince-fugnitto Do you mean that we normalize all root URIs in WorkspaceService? It sounds even better.

[vince-fugnitto]

@vince-fugnitto Do you mean that we normalize all root URIs in WorkspaceService? It sounds even better.

Yes exactly, it is what @corneliusludmann mentioned in his pull-request description:

An alternative would be to fix the roots in the workspace service directly so that it benefits everywhere.

[akosyakov]

@corneliusludmann Can you do alternative solution? It sounds great.

[corneliusludmann]

Sure. I'll do this.

[corneliusludmann]

I'm not sure which is the best place to put this. The call chain is:

• computeRoots()
• toValidRoot()
• toFileStat()

Is normalizing the path in toFileStat() too aggressive? I think it fits very well in this method. However, it is called from 4 other places in workspace-service.ts. I think, they all benefit form this change but if you afraid of side effects I would add this to toValidRoot() instead. WDYT @akosyakov?

theia/packages/workspace/src/browser/workspace-service.ts

Lines 418 to 438 in 751a3d7

	/**
	* returns a FileStat if the argument URI points to a file or directory. Otherwise, `undefined`.
	*/
	protected async toFileStat(uri: URI | string | undefined): Promise<FileStat | undefined> {
	if (!uri) {
	return undefined;
	}
	let uriStr = uri.toString();
	try {
	if (uriStr.endsWith('/')) {
	uriStr = uriStr.slice(0, -1);
	}
	const fileStat = await this.fileSystem.getFileStat(uriStr);
	if (!fileStat) {
	return undefined;
	}
	return fileStat;
	} catch (error) {
	return undefined;
	}
	}

[akosyakov]

It seems to be fine. @vince-fugnitto Could you verify it please?

[vince-fugnitto]

It seems to be fine. @vince-fugnitto Could you verify it please?

Sure, I'll test it!
@corneliusludmann would you mind adding tests to the workspace-service to verify if we now correctly handle normalizing? If not I can definitely do so :)

@corneliusludmann do you mind updating the workspace-service tests for toFileStat to verify if we are correctly normalizing uris?

[corneliusludmann]

@vince-fugnitto I updated the PR with a test. I hope that's of some use. Feel free to modify or to propose changes.

[vince-fugnitto]

I tested the changes (terminal in multi-root) and it works correctly!
Thank you for taking care of the pull-request and also including a test case :)