Skip to content

Commit

Permalink
Delete local storage patch
Browse files Browse the repository at this point in the history 
The main goal of this patch was to make user settings stored on disk
instead of in the browser, but this stopped working some time ago.  Not
only that but it is causing a bug where a workspace will not fully open.

A secondary goal was to fix the Vim extension but the extension appears
to work just fine without this change now (both the server and browser
versions).

This patch is not useful anymore anyway because there are remote-level
settings that *do* get stored on disk and can be used instead of
user-level settings when necessary.

Fixes #3061, and possibly #6153.
  • Loading branch information
@code-asher
code-asher committed Nov 20, 2023
1 parent 958c520 commit 09dd5fe
Show file tree
Hide file tree
Showing 5 changed files with 15 additions and 82 deletions.
16 changes: 8 additions & 8 deletions patches/disable-downloads.diff
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@ Index: code-server/lib/vscode/src/vs/workbench/browser/web.api.ts
===================================================================
--- code-server.orig/lib/vscode/src/vs/workbench/browser/web.api.ts
+++ code-server/lib/vscode/src/vs/workbench/browser/web.api.ts
@@ -281,6 +281,11 @@ export interface IWorkbenchConstructionO
@@ -276,6 +276,11 @@ export interface IWorkbenchConstructionO
*/
readonly userDataPath?: string
readonly configurationDefaults?: Record<string, any>;

+ /**
+ * Whether the "Download..." option is enabled for files.
Expand All @@ -40,9 +40,9 @@ Index: code-server/lib/vscode/src/vs/workbench/services/environment/browser/envi
* Gets whether a resolver extension is expected for the environment.
*/
readonly expectsResolverExtension: boolean;
@@ -111,6 +116,13 @@ export class BrowserWorkbenchEnvironment
return this.options.userDataPath;
}
@@ -110,6 +115,13 @@ export class BrowserWorkbenchEnvironment
@memoize
get cacheHome(): URI { return joinPath(this.userRoamingDataHome, 'caches'); }

+ get isEnabledFileDownloads(): boolean {
+ if (typeof this.options.isEnabledFileDownloads === "undefined") {
Expand All @@ -52,7 +52,7 @@ Index: code-server/lib/vscode/src/vs/workbench/services/environment/browser/envi
+ }
+
@memoize
get argvResource(): URI { return joinPath(this.userRoamingDataHome, 'argv.json'); }
get workspaceStorageHome(): URI { return joinPath(this.userRoamingDataHome, 'workspaceStorage'); }

Index: code-server/lib/vscode/src/vs/server/node/serverEnvironmentService.ts
===================================================================
Expand All @@ -78,10 +78,10 @@ Index: code-server/lib/vscode/src/vs/server/node/webClientServer.ts
===================================================================
--- code-server.orig/lib/vscode/src/vs/server/node/webClientServer.ts
+++ code-server/lib/vscode/src/vs/server/node/webClientServer.ts
@@ -332,6 +332,7 @@ export class WebClientServer {
@@ -331,6 +331,7 @@ export class WebClientServer {
const workbenchWebConfiguration = {
remoteAuthority,
webviewEndpoint: vscodeBase + this._staticRoute + '/out/vs/workbench/contrib/webview/browser/pre',
userDataPath: this._environmentService.userDataPath,
+ isEnabledFileDownloads: !this._environmentService.args['disable-file-downloads'],
_wrapWebWorkerExtHostInIframe,
developmentOptions: { enableSmokeTestDriver: this._environmentService.args['enable-smoke-test-driver'] ? true : undefined, logLevel: this._logService.getLevel() },
Expand Down
4 changes: 2 additions & 2 deletions patches/display-language.diff
View file
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ Index: code-server/lib/vscode/src/vs/server/node/webClientServer.ts
import { CharCode } from 'vs/base/common/charCode';
import { getRemoteServerRootPath } from 'vs/platform/remote/common/remoteHosts';
import { IExtensionManifest } from 'vs/platform/extensions/common/extensions';
@@ -344,6 +345,8 @@ export class WebClientServer {
@@ -343,6 +344,8 @@ export class WebClientServer {
callbackRoute: this._callbackRoute
};

Expand All @@ -229,7 +229,7 @@ Index: code-server/lib/vscode/src/vs/server/node/webClientServer.ts
const nlsBaseUrl = this._productService.extensionsGallery?.nlsBaseUrl;
const values: { [key: string]: string } = {
WORKBENCH_WEB_CONFIGURATION: asJSON(workbenchWebConfiguration),
@@ -352,6 +355,7 @@ export class WebClientServer {
@@ -351,6 +354,7 @@ export class WebClientServer {
WORKBENCH_NLS_BASE_URL: vscodeBase + (nlsBaseUrl ? `${nlsBaseUrl}${!nlsBaseUrl.endsWith('/') ? '/' : ''}${this._productService.commit}/${this._productService.version}/` : ''),
BASE: base,
VS_BASE: vscodeBase,
Expand Down
10 changes: 5 additions & 5 deletions patches/getting-started.diff
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ Index: code-server/lib/vscode/src/vs/workbench/browser/web.api.ts
===================================================================
--- code-server.orig/lib/vscode/src/vs/workbench/browser/web.api.ts
+++ code-server/lib/vscode/src/vs/workbench/browser/web.api.ts
@@ -286,6 +286,11 @@ export interface IWorkbenchConstructionO
@@ -281,6 +281,11 @@ export interface IWorkbenchConstructionO
*/
readonly isEnabledFileDownloads?: boolean

Expand Down Expand Up @@ -163,7 +163,7 @@ Index: code-server/lib/vscode/src/vs/workbench/services/environment/browser/envi
* Gets whether a resolver extension is expected for the environment.
*/
readonly expectsResolverExtension: boolean;
@@ -123,6 +128,13 @@ export class BrowserWorkbenchEnvironment
@@ -122,6 +127,13 @@ export class BrowserWorkbenchEnvironment
return this.options.isEnabledFileDownloads;
}

Expand All @@ -175,7 +175,7 @@ Index: code-server/lib/vscode/src/vs/workbench/services/environment/browser/envi
+ }
+
@memoize
get argvResource(): URI { return joinPath(this.userRoamingDataHome, 'argv.json'); }
get workspaceStorageHome(): URI { return joinPath(this.userRoamingDataHome, 'workspaceStorage'); }

Index: code-server/lib/vscode/src/vs/server/node/serverEnvironmentService.ts
===================================================================
Expand All @@ -201,9 +201,9 @@ Index: code-server/lib/vscode/src/vs/server/node/webClientServer.ts
===================================================================
--- code-server.orig/lib/vscode/src/vs/server/node/webClientServer.ts
+++ code-server/lib/vscode/src/vs/server/node/webClientServer.ts
@@ -335,6 +335,7 @@ export class WebClientServer {
@@ -334,6 +334,7 @@ export class WebClientServer {
remoteAuthority,
webviewEndpoint: vscodeBase + this._staticRoute + '/out/vs/workbench/contrib/webview/browser/pre',
userDataPath: this._environmentService.userDataPath,
isEnabledFileDownloads: !this._environmentService.args['disable-file-downloads'],
+ isEnabledCoderGettingStarted: !this._environmentService.args['disable-getting-started-override'],
_wrapWebWorkerExtHostInIframe,
Expand Down
66 changes: 0 additions & 66 deletions patches/local-storage.diff
View file

This file was deleted.

1 change: 0 additions & 1 deletion patches/series
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ logout.diff
store-socket.diff
proxy-uri.diff
unique-db.diff
local-storage.diff
service-worker.diff
sourcemaps.diff
disable-downloads.diff
Expand Down

11 comments on commit 09dd5fe

@benz0li
Copy link
Contributor

Choose a reason for hiding this comment

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

@code-asher Your statement:

The main goal of this patch was to make user settings stored on disk instead of in the browser, but this stopped working some time ago.

My response (Dec 11, 2021):

I don't mind that the state is stored in the browser's storage (#4212).

But one should have the possibility to pre-populate ~/.local/share/code-server/User/settings.json with default settings (#2274 (comment)), e.g. when creating a docker image.

Originally posted by @benz0li in #4609 (comment)

@benz0li
Copy link
Contributor

Choose a reason for hiding this comment

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

@code-asher Your statement:

This patch is not useful anymore anyway because there are remote-level settings that do get stored on disk and can be used instead of user-level settings when necessary.

IMHO Remote settings cannot fully replace user settings.

Some extension's settings can only be stored as user setting but not as remote setting.
ℹ️ At least this was the case at the end of 2021. Maybe that has changed in the meantime.

@benz0li
Copy link
Contributor

Choose a reason for hiding this comment

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

Fixes #3061, and possibly #6153.

I was able to reproduce #3061 but never ran into #6153.

@benz0li
Copy link
Contributor

Choose a reason for hiding this comment

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

@code-asher I will check, whether all [my] settings can be fully migrated from ~/.local/share/code-server/User/settings.json to ~/.local/share/code-server/Machine/settings.json or not.

This change would definitely cause the following: If a user clears the browser's cache, the user settings will be lost.

@benz0li
Copy link
Contributor

Choose a reason for hiding this comment

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

I will check, whether all [my] settings can be fully migrated from ~/.local/share/code-server/User/settings.json to ~/.local/share/code-server/Machine/settings.json or not.

The only setting that cannot be migrated is "telemetry.telemetryLevel": "off".
👉 So I will use the --disable-telemetry flag to disable telemetry by default.

@benz0li
Copy link
Contributor

Choose a reason for hiding this comment

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

Long story short: code-server works just fine after deleting the local storage patch.

Migrating to Remote Settings (~/.local/share/code-server/Machine/settings.json) seems possible.

@benz0li
Copy link
Contributor

@benz0li benz0li commented on 09dd5fe Nov 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@code-asher If one opens Settings, the 'User' tab is selected by default. With the current behaviour1, this should default to 'Remote'.

Footnotes

  1. 'Folder settings' overwrite 'Workspace settings' overwrite 'Remote settings' (Machine settings) overwrite 'User settings' overwrite 'Default settings' ↩

@benz0li
Copy link
Contributor

Choose a reason for hiding this comment

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

If one opens Settings, the 'User' tab is selected by default.

Codespaces behave the same way.

With the current behaviour, this should default to 'Remote'.

There is no need for code-server to behave differently.

@code-asher
Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for testing all this and confirming the Codespaces behavior!

The only setting that cannot be migrated is "telemetry.telemetryLevel": "off".

How strange that they only allow this to be a user-level setting. 🤔

@benz0li
Copy link
Contributor

Choose a reason for hiding this comment

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

How strange that they only allow this to be a user-level setting. 🤔

It says This setting has an application scope and can be set only in the user settings file.

@code-asher
Copy link
Member Author

Choose a reason for hiding this comment

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

Ah I see, and I see security settings can only be in user settings as well. How unfortunate, that does make this not quite the perfect replacement.

Please sign in to comment.