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

Path conversion for windows and posix file systems #10591

Merged
merged 2 commits into from
Apr 20, 2022

Conversation

msujew
Copy link
Member

@msujew msujew commented Jan 4, 2022

What it does

Closes #10552

Enables to convert URIs in the frontend to a file system path independent of the frontend OS (unlike vscode-uri.fsPath). This can be used to display backend paths, such as the file paths of opened editors in the title property.

How to test

This one is a bit difficult to test. The easiest setup would be to run Theia on a Unix machine and access the browser version on Windows.


  1. Open a file in the editor. The title (hover display) of the file node should always display the correct backend path. I.e. C:\file\to\path.txt on windows and /path/to/file.txt on Unix.

  1. Start Theia on a different OS than your frontend OS.
  2. Download and install this extension (source)
  3. Open the context-menu for package.json files which contain a dependencies section. The Install Dependencies menu entry should be visible.
  4. Confirm that other configurations (electron/windows/unix) work as expected

Review checklist

Reminder for reviewers

@msujew
Copy link
Member Author

msujew commented Jan 4, 2022

cc @bd82 @ChayaLau Since you were part of the discussion in #10552, I would like you to have a look at this, and whether it fixes your issue. It works on my end at least :)

@msujew msujew added the filesystem issues related to the filesystem label Jan 4, 2022
@msujew msujew force-pushed the msujew/posix-windows-path branch from 6822bf1 to da93ba2 Compare January 4, 2022 16:50
@bd82
Copy link
Contributor

bd82 commented Jan 5, 2022

Thanks @msujew

At the moment we have a workaround and also had the internal feature that needed the fix/workaround reduced in priority
So I am unsure when we will get around to testing it 😢

@msujew
Copy link
Member Author

msujew commented Jan 5, 2022

@bd82 Not a problem, I actually tackled this issue due to something different that I noticed, it just also fixes the issue you were experiencing ;)

this.resource = this.contextKeyService.createKey<Uri>('resource', undefined);
this.resourceSchemeKey = this.contextKeyService.createKey<string>('resourceScheme', undefined);
this.resourceFileName = this.contextKeyService.createKey<string>('resourceFilename', undefined);
this.resourceExtname = this.contextKeyService.createKey<string>('resourceExtname', undefined);
this.resourceLangId = this.contextKeyService.createKey<string>('resourceLangId', undefined);
this.resourceDirName = this.contextKeyService.createKey<string>('resourceDirName', undefined);
this.resourcePath = this.contextKeyService.createKey<string>('resourcePath', undefined);
this.isPosix = await this.applicationService.getBackendOS() !== OS.Type.Windows;
Copy link
Contributor

Choose a reason for hiding this comment

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

@msujew, it looks like you could import isWindows from core/src/common/os.ts rather than relying on the ApplicationServer. That would give you what you need synchronously rather than waiting on a Promise.

Copy link
Member

Choose a reason for hiding this comment

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

Except isWindows will give the OS of the running browser here, and not the remote backend? Not sure which one makes more sense here, but they aren't the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

@colin-grant-work Exactly as @paul-marechal is saying, in this case we need the backend OS, since it specifies the string format of all file paths (forward- vs. backward-slashes, drive letters in front, etc.). The whole reason why this bug exists, is that the frontend and backend might convert the same URI into different strings, which would then result in a mismatch of the when context property of commands.

In particular, when we have something like resourcePath in ext:pkgJsonWithTestScript as a context, the monaco frontend on windows will convert the current resourcePath into something like \user\settings.json, while the posix backend path is actually using /user/settings.json. Monaco will then perform an in check on the ext:pkgJsonWithTestScript object, which will fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose then I wonder whether we shouldn't make retrieving information about the backend OS part of the pre-load startup routine so that we can then access it synchronously from inside the application? That would alleviate some of my anxiety about race conditions.

@colin-grant-work colin-grant-work self-requested a review January 27, 2022 15:55
@msujew msujew force-pushed the msujew/posix-windows-path branch from da93ba2 to 2dc1c78 Compare March 2, 2022 14:09
@msujew
Copy link
Member Author

msujew commented Mar 2, 2022

@colin-grant-work Care to take another look at this? I also opened #10830 to keep track of all misusage of URI#path.toString().

@colin-grant-work
Copy link
Contributor

@msujew, I've got a brand-new Windows system I should be able to test this on, so I'll take another look today :-)

Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

The changes work for me in various combinations of Linux and Windows servers and browsers. My only comment is about the asynch retrieval of the backend OS, which is probably fine but could potentially go wrong. If it's impractical to make the check synchronous while the application is running, then I'm fine with the current implementation.

@msujew msujew force-pushed the msujew/posix-windows-path branch from 85b7178 to 2616ae7 Compare April 11, 2022 14:30
@msujew
Copy link
Member Author

msujew commented Apr 11, 2022

@colin-grant-work Alright, I did some larger refactoring for getBackendOS(), and now we preload the backend OS in the same step we also load localizations. That allows for sync access on the backend type. Is that more how you imagine it should work?

@msujew msujew requested a review from colin-grant-work April 11, 2022 14:33
@colin-grant-work
Copy link
Contributor

@msujew, I missed this review request - sorry; I'll take a look today.

Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

The functionality continues to work, and the move to a preloader makes this code cleaner, cleans up a few other areas, and opens up a lot of possibilities (e.g. preferences) so that's a definite 👍. One or two minor things that you can address or leave, as you choose.

packages/core/src/browser/resource-context-key.ts Outdated Show resolved Hide resolved
packages/core/src/common/path.ts Outdated Show resolved Hide resolved
@msujew msujew force-pushed the msujew/posix-windows-path branch from 2616ae7 to 4491930 Compare April 20, 2022 14:30
@msujew msujew force-pushed the msujew/posix-windows-path branch from 4491930 to 2f65949 Compare April 20, 2022 20:09
@msujew msujew merged commit 5e5c3b5 into master Apr 20, 2022
@msujew msujew deleted the msujew/posix-windows-path branch April 20, 2022 21:23
@github-actions github-actions bot added this to the 1.25.0 milestone Apr 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
filesystem issues related to the filesystem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Strange problem with in conditional operator
4 participants