search

material-theme / vsc-material-theme

Material Theme, the most epic theme for Visual Studio Code
https://www.material-theme.dev
Other
11.21k stars 788 forks source link

use globalStorageUri for user settings #1295

Closed Stunkymonkey closed 4 months ago

Stunkymonkey commented 8 months ago

fix for #1294

this might be a breaking change!?

Stunkymonkey commented 8 months ago

on NixOS I already submitted a patch for this: https://github.com/NixOS/nixpkgs/pull/291021/commits/a00e878cb349df2837f4c746f61aeb5ddf4905ca So this is already tested by me and others.

LasaleFamine commented 8 months ago

Hi @Stunkymonkey thanks for the PR. I think this indeed would be a breaking change since this.userConfigFileUri is used to load and write the user configuration, the old one won't be migrate with only this change. You can see the references: https://github.com/Stunkymonkey/vsc-material-theme/blob/use-globalStorageUri-for-settings/src/core/extension-manager.ts#L121 https://github.com/Stunkymonkey/vsc-material-theme/blob/use-globalStorageUri-for-settings/src/core/extension-manager.ts#L91

I think it would be good to check if the old one is present and if so switch the location to the new one, and the check can be removed when we release a major version. cc @equinusocio

Stunkymonkey commented 8 months ago

@LasaleFamine I have no development setup locally, so I am coding blind... without testing it. Be warned.

So I added some more to check for the old file and if existent use it, else use the new one.

Is this what you had in mind?

PS: there might be await missing. not sure.

LasaleFamine commented 8 months ago

@Stunkymonkey actually in this way we are considering the old file as something to use still. If the objective is to migrate to the new use config location, we should copy the old file if present in the new location. If the configuration can work either in both locations maybe this change can be ok.

Stunkymonkey commented 8 months ago

@LasaleFamine so you prefer this new solution? Or do you want a combination of both?

LasaleFamine commented 8 months ago

@Stunkymonkey thanks for the change. This feels to me the correct way to change the config file. Did you have the possibility to test it?

Stunkymonkey commented 8 months ago

@Stunkymonkey thanks for the change. This feels to me the correct way to change the config file. Did you have the possibility to test it?

Sadly no. I do not even have npm installed. Is it possible you go on from here?

equinusocio commented 7 months ago

Please setup the whole environment to work and debug this extension. There are many typescript errors.

equinusocio commented 4 months ago

There are still typescript error

Stunkymonkey commented 4 months ago

could you please take over the PR? I do not have time to setup the dev-env.