Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Make Theia data folder name configurable #6650
Make Theia data folder name configurable #6650
Changes from all commits
e5bdddb
18f81b6
3d1e3bf
eb03d19
ee496c4
7a4a762
c9bed13
86983cc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API change is odd.
Let's put aside, it was sync and now it is async, but earlier I could do
Now I have to do
You ignored the default values. I guess we cannot avoid a breaking API change, then let's have a better API. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid passing of empty string we may default the argument to it. Just missed the usage. My PR changes many things...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need the app data folder? Is it required to comply with VS Code APIs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Windows VSCode uses AppData folder for some info.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is for VS Code compatibility, but we should understand it is better. I don't think it is something Windows or VS Code specific.
For instance, Electron distinguishes between the app folder (
app.getPath('appData')
) and the user folder (app.getPath('userData')
), see https://github.com/electron/electron/blob/master/docs/api/app.md#appgetpathname. If I understand correctly former is used for place where program stores its files and second one where a program stores user files. If it is true, we should put it like that and list examples of files in comments.@thegecko @mcgordonite Maybe you can shed some light on this question :) I think this PR is important for you, it is cleanly separates how to configure user folder for the concrete product.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please link the corresponding VS Code source? I couldn't find it with a text search for
getAppDataFolder
in thevscode
repo.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
userData
folder is simply theappData
folder with the current application name at the end. It's not Windows specific.e.g. on MacOS:
appData: ~/Library/Application Support
userData: ~/Library/Application Support/Mbed Studio (for example)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking that we use
userData
here bogusly. We probably should name it differently, user data it like where local storage (app state specific to user) is stored, not there settings or recent workspaces are stored.Maybe we should call it
appHome
folder instead to avoid confusions withuserData
meaning from Electron? So there would be 4 folders:Does it make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mmorhun I'm a bit confused, I could not find a single client making use of
getAppDataFolder
. Did I overlook something?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's why I have asked: why do we need it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's keep a naming as it's and add js-docs:
userDataFolder
controls a folder where user data like settings, recent workspaces and plugins are storedappDataFolder
controls a folder where app data are stored, like plugin logs and storagesbtw
PluginPathsServiceImpl.getLogsDirPath
andPluginPathsServiceImpl.getWorkspaceStorageDirPath
are bogus, they should usegetAppDataFolder
, notgetUserDataFolder
Also
getAppDataFolder
implementation should be aligned with https://github.com/microsoft/vscode/blob/f3b191ab38fb9f1717ce5ce3396bb41204ffb399/src/paths.js#L18-L25, except bits of VSCODE_DATA, APP_DATA is Electron variable andUSERPROFILE
is windows variable, using homedir always is bogusThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've not seen it used, but you could imagine a company with multiple apps potentially sharing config data between them there?
I would be inclined to keep it and name it how an electron user would expect to see it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The two
await
s do not depend on each other:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, please check and tell me if I have overlooked something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, will fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure why new APIs are necessary. Do you implement some VS Code API with them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably I changed formatting or so for this line. It was added long time ago.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean, there are no such APIs on master
theia/packages/plugin-ext/src/common/plugin-api-rpc.ts
Lines 993 to 996 in 4d4f6c2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My mistake, sorry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remote Theia APIs accept URIs, not paths