Make Theia data folder name configurable #6650
Conversation
|
The point is to use |
|
Yes they do. For example here. |
But they are already aligned it, i.e. |
packages/userstorage/src/browser/user-storage-service-filesystem.ts
Outdated
Show resolved
Hide resolved
packages/plugin-ext/src/main/node/paths/plugin-paths-service.ts
Outdated
Show resolved
Hide resolved
I think, the case with using of another folder is handled by using the constant. |
Then it does not address #4488 If i rename
We should care about the default implementation. Performance issues of custom user data storage it is extenders' problems. |
It will work, just Theia will use another folder for its data (which means previous settings are unused). |
Agree. It should be possible. As for now, such an option is the hardcoded constant. So is it ok for you to have rebindable value for this constant to resolve the issue? |
|
@akosyakov it would be nice to have some agreement on this issue.
|
|
I might be missing something here, but moving the constant "PluginPaths.THEIA_DIR" to "THEIA_USER_STORAGE_FOLDER" does not change anything |
Let's improve our
We should review the code in Theia repo where we use |
|
I've updated the PR according to our discussion above. |
mmorhun
changed the title
|
@mmorhun that's really good changes to enable installing different theia products next to each other! Please look at comment though, it would be good to make it less breaking in some places. @elaihau @RomanNikitenko please check whether breaking in the task extension can cause any issue, at first look it is fine |
packages/userstorage/src/browser/user-storage-service-filesystem.ts
Outdated
Show resolved
Hide resolved
The impacted feature that I can think of is I tested in the example browser. |
Signed-off-by: Mykola Morhun <mmorhun@redhat.com>
|
The PR is ready to be tested again. |
There was a problem hiding this comment.
@mmorhun
I tested the changes after your update and found that unfortunately the plugins can not be started for me...
I tested your changes as is, without overriding .theia folder.
Could you check it, can you reproduce it?
thanks in advance!
| async createUri(folder: URI, configPath: string, configName: string = this.getConfigName()): Promise<URI> { | ||
| if (!configPath) { |
There was a problem hiding this comment.
This API change is odd.
Let's put aside, it was sync and now it is async, but earlier I could do
pc.createUri(myFolderUri)
Now I have to do
pc.createUri(myFolderUri, '') // Note the empty string
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.
To avoid passing of empty string we may default the argument to it. Just missed the usage. My PR changes many things...
| dataFolderUriBuilder.resolve(WINDOWS_APP_DATA_DIR); | ||
| dataFolderUriBuilder.resolve(WINDOWS_ROAMING_DIR); | ||
| } | ||
| dataFolderUriBuilder.resolve(await this.getDataFolderName()); |
There was a problem hiding this comment.
Same as above, please check and tell me if I have overlooked something.
There was a problem hiding this comment.
You are right, will fix
| } | ||
|
|
||
| async getUserDataFolder(): Promise<string> { | ||
| return FileUri.create(await this.getUserHomeFolder()).resolve(await this.getDataFolderName()).toString(); |
There was a problem hiding this comment.
The two awaits do not depend on each other:
async getUserDataFolder(): Promise<string> {
const [home, data] = await Promise.all([
this.getUserHomeFolder(),
this.getDataFolderName()
]);
return FileUri.create(home).resolve(data).toString();
}| getDataFolderName(): Promise<string> | ||
| getUserDataFolder(): Promise<string> | ||
| /** Windows specific. Returns system data folder of Theia. On other than Windows systems is the same as getUserDataFolder */ | ||
| getAppDataFolder(): Promise<string> |
There was a problem hiding this comment.
Why do we need the app data folder? Is it required to comply with VS Code APIs?
There was a problem hiding this comment.
On Windows VSCode uses AppData folder for some info.
There was a problem hiding this comment.
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.
Yes, it is for VS Code compatibility
Can you please link the corresponding VS Code source? I couldn't find it with a text search for getAppDataFolder in the vscode repo.
There was a problem hiding this comment.
userData folder is simply the appData 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.
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 with userData meaning from Electron? So there would be 4 folders:
- userHome: ~
- appHome = user home + home folder name: ~/.theia
- appData: ~/Library/Application Support
- userData = appData + productName: ~/Library/Application Support/Mbed Studio (for example)
Does it make sense?
There was a problem hiding this comment.
@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.
I could not find a single client making use of
getAppDataFolder
That's why I have asked: why do we need it.
There was a problem hiding this comment.
let's keep a naming as it's and add js-docs:
userDataFoldercontrols a folder where user data like settings, recent workspaces and plugins are storedappDataFoldercontrols a folder where app data are stored, like plugin logs and storages
btw PluginPathsServiceImpl.getLogsDirPath and PluginPathsServiceImpl.getWorkspaceStorageDirPath are bogus, they should use getAppDataFolder, not getUserDataFolder
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 and USERPROFILE is windows variable, using homedir always is bogus
There was a problem hiding this comment.
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.
Signed-off-by: Mykola Morhun <mmorhun@redhat.com>
| import { PLUGIN_RPC_CONTEXT, EnvMain } from '../common'; | ||
| import { EnvVariablesServer, EnvVariable } from '@theia/core/lib/common/env-variables'; | ||
|
|
||
| export class EnvVariablesServerExt implements EnvVariablesServer { |
There was a problem hiding this comment.
Why does in plugin code needs to implement theia core apI? I see that many new methods are not used to implement theia.d.ts. Please remove it does not help supporting VS Code APIs.
There was a problem hiding this comment.
We've added some new methods into Theia APIs over VSCode APIs. They were required for some Theia plugins features. Does having extra APIs hurt?
As about my changes. Yes, I added new methods which are not used in theia.d.ts implementation, but they are needed to comply with EnvVariablesServer interface. I believe, that refactoring of this is out of scope for this PR.
There was a problem hiding this comment.
We have conventions that main and ext shapes should be defined in the plugin api protocol file, here we don't follow it but trying to reuse code from core which can pull core deps in plugin host process, in additional we add methods which are not needed. cc @benoitf @tsmaeder
i.e. there should be EnvExt interface which only has methods required to implement EnvMain. core APIs should not be used in the plugin host process. Then it will be consistent with design of all other plugin shapes without any unnecessary code.
There was a problem hiding this comment.
@akosyakov I completely agree with you. What I am telling is that such refactoring is out of scope of this PR. I prefer to have a PR which fixes one specific issue. Otherwise we will have a mess in PR, long terms PRs and the process of contributing is getting too long.
There was a problem hiding this comment.
I think such refactoring makes the PR smaller, since you remove a lot of code instead of adding new code trying to save the bogus approach. It will be out of scope if you won't change this file or protocol, but you did already.
| @@ -992,7 +993,13 @@ export interface DocumentsMain { | |||
|
|
|||
| export interface EnvMain { | |||
| $getEnvVariable(envVarName: string): Promise<string | undefined>; | |||
| $getAllEnvVariables(): Promise<EnvVariable[]> | |||
There was a problem hiding this comment.
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.
Probably I changed formatting or so for this line. It was added long time ago.
There was a problem hiding this comment.
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.
My mistake, sorry.
| const globalStorageFolderPath = new Path(userDataFolderPath).join('globalStorage').toString(); | ||
|
|
||
| // Make sure that folder by the path exists | ||
| if (! await this.fileSystem.exists(globalStorageFolderPath)) { |
There was a problem hiding this comment.
remote Theia APIs accept URIs, not paths
|
@RomanNikitenko thanks for testing. When I tested this PR a few days ago it was working with a plugin. When I tested it today, I had the same error you had... weird... |
| @@ -450,7 +454,7 @@ export class PreferencesEditorsContainer extends DockPanel { | |||
|
|
|||
| let uri = preferenceUri; | |||
| if (preferenceUri.scheme === UserStorageUri.SCHEME && homeUri) { | |||
| uri = homeUri.resolve(THEIA_USER_STORAGE_FOLDER).resolve(preferenceUri.path); | |||
| uri = homeUri.resolve(await this.envServer.getDataFolderName()).resolve(preferenceUri.path); | |||
There was a problem hiding this comment.
we should get rid of homeUri here and use getUserDataFolder instead, homeUri breaks abstraction by assuming that user home is this.fileSystem.getCurrentUserHome in EnvVariableServer
There was a problem hiding this comment.
I wonder should we deprecate FileSystem.getCurrentUserHome in favour of EnvVariableServer.getUserHomeFolder. I think it would be more correct conceptually and prohibit breaking the abstraction as above.
There was a problem hiding this comment.
let's deprecate, it does not make sense to have 2 APIs for the same thing. No need to change clients right now, just add @deprecate annotation which says that EnvVariableServer.getUserHomeFolder should be used instead
| @injectable() | ||
| export class UserStorageServiceFilesystemImpl implements UserStorageService { | ||
|
|
||
| protected readonly toDispose = new DisposableCollection(); | ||
| protected readonly onUserStorageChangedEmitter = new Emitter<UserStorageChangeEvent>(); | ||
| protected readonly userStorageFolder: Promise<URI | undefined>; |
There was a problem hiding this comment.
this line should be reverted, readonly is correct, variable is assigned in the constructor
|
Guys, I'm not sure that I will be able to address all your comments before release. I had well tested solution, but all these new remarks require some additional time that I don't have at the moment... |
|
Thanks for the heads-up, @mmorhun. No worries, you have already taken care of the significant part of this task 👍, do you want me to continue on the top of your work? Give me the green lights, and I can assist you a bit. |
|
@mmorhun thank you for all great job on this PR anyway! It is fine to collaborate and work together to get stuff done. I think it would be fine if @kittaakos push an additional commit to address remaining comments. As usual authorship of the original commit should be preserved: https://github.com/eclipse-theia/theia/blob/master/doc/pull-requests.md#completing-pr |
|
Thank you guys for good review and spotting issue in this PR. I don't mind if some of you continue this work. Thank you! |
|
@mmorhun I've noticed that you resolved some conversations. Does it mean you are going to push once more or we can unresolve them? |
|
To sum up:
This is not a solution for non-bundled electron applications, as the application name is always Edit: I have added this item to our forthcoming dev-meeting: https://github.com/eclipse-theia/theia/wiki/Dev-Meetings#agenda-2020-02-18 |
See above 👆 CC @thegecko |
|
@akosyakov my changes are trivial, so no value of pushing them now. But I've unresolved conversations. |
@kittaakos We should figure out though why VS Code does not want to unpack extensions under app data folder as well, but always under user home. It would be important for support of VS Code extensions in Electron, i.e. on install they should be unpacked somewhere and loaded on next start from there as well. Not something for this PR, but we should know why at least. |
Really not good :( If it has to be done, we should at least get it in before v1. |
Here is the answer:
From the
Where the user-specific data files is the important part. No extensions, just config files. |
|
It's turned out that VS Code strategy does not follow platform guidance and cause various issues: microsoft/vscode#3884 (comment) For them it is hard already to migrate for us it would be good time to learn and make it properly. |
|
@kittaakos JFYI, I use Fedora (31 as of now) but both |
Signed-off-by: Mykola Morhun mmorhun@redhat.com
What it does
Makes Theia data folder configurable, so it could be redefined to another than
.theia.Resolves #4488
Review checklist
Reminder for reviewers