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

Introduced config dir on the env variable server #7214

Merged
merged 2 commits into from
Feb 26, 2020
Merged

Introduced config dir on the env variable server #7214

merged 2 commits into from
Feb 26, 2020

Conversation

kittaakos
Copy link
Contributor

@kittaakos kittaakos commented Feb 25, 2020

Follow-up of #6650.

What it does

From now on, all configuration (settings.json, keymaps.json, etc) and VS Code extension things (such as globalStorage, logs, and plugin-storage) goes directly under the configuration folder.
Downstream projects can override the default, .theia folder name for the configurations. See the example here.

What it does not do:

  • we do not distinguish between cachable (XDG_CACHE_HOME on Linux and AppData/Local on Windows, such as workspace storage, extensions and their logs for example) and shareable things (XDG_CONFIG_HOME on Linux and AppData/Roaming on WIndows, such as settings.json, keymaps.json)

How to test

Try the example customization:

  • try to run a task from a workspace location (note, tasks.json should go under .theia-example),
  • see you have the .theia-example folder under ~/,
  • POSIX: you can open recent workspaces and see if the paths are still correctly tildyfied.

Review checklist

Reminder for reviewers

@kittaakos
Copy link
Contributor Author

Note, once the review is over, I am going to drop 8ab1980 from this PR.

@@ -54,6 +54,9 @@ export namespace FileUri {
return fsPathFromVsCodeUri + '\\';
}
}
if (fsPathFromVsCodeUri.startsWith('/file:')) {
Copy link
Member

Choose a reason for hiding this comment

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

It looks weird, fsPath should not have scheme already. How can I get URI leading to such case. I think some clients are likely to be bogus.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea, I reused the previous changes.

Copy link
Member

Choose a reason for hiding this comment

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

@mmorhun Could you give a hint? It looks like there is a bug in how some client constructs URI. We should not expect such fs paths.

Copy link
Member

Choose a reason for hiding this comment

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

let's remove it, if there will be a regression, we will fix it properly.

Copy link
Contributor

Choose a reason for hiding this comment

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

@akosyakov I added that code as a quick fix. I agree that this is a kludge and we have to find proper solution. The reason I added it was that FileUri.fsPath, was returning wrong path: /file:/right/path.

Copy link
Member

Choose a reason for hiding this comment

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

@mmorhun it can happen only if a uri passed to FileUri.fsPath is bogus, the real issue is to figure how one get such bogus URI and fix the producer

@akosyakov
Copy link
Member

@kittaakos please squash not your commits in one commit, all of them are fixing issues in the original commit. It is alright since the authorship will be preserved.

@akosyakov akosyakov added the core issues related to the core of the application label Feb 25, 2020
@kittaakos kittaakos force-pushed the GH-4488 branch 4 times, most recently from 4c2ad5e to 829839e Compare February 26, 2020 08:07
@kittaakos kittaakos force-pushed the GH-4488 branch 2 times, most recently from f09eeea to 17d408f Compare February 26, 2020 08:58
@kittaakos kittaakos force-pushed the GH-4488 branch 3 times, most recently from 607972f to c6aefc0 Compare February 26, 2020 09:58
@kittaakos
Copy link
Contributor Author

@akosyakov, I have fixed the build, the tests, and updated the changelog. This PR is ready for review.

CC: @benoitf

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

It works well and thank you for cleaning up everything!

I've tried to change between .my-theia and .theia several times with different set of core and plugin configurations and it worked nicely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core issues related to the core of the application
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants