Hot exit empty workspace support by Tyriar · Pull Request #16303 · microsoft/vscode

Tyriar · 2016-12-01T02:31:13Z

Tyriar · 2016-12-01T02:35:28Z

@bpasero I managed to get empty workspaces working, it's just super rough. I'd love some feedback on the general approach and position of things, specifically:

Passing vscodeWindowId around inside IWindowConfiguration
Including vscodeWindowId in IEnvironmentService
The name of vscodeWindowId (it needs to be distinct from VSCodeWindow.id)
The format of vscodeWindowId: Date.now().toString(). I was also considered crypto.randomBytes (might be too slow) and Math.random() (overkill if Date.now() can guarantee uniqueness)
The handling of emptyToOpen

I'll be polishing this tomorrow and will probably stay up so we can get this in before the weekend.

bpasero · 2016-12-01T16:20:32Z

 	extensionDevelopmentPath?: string;
 	allowFullscreen?: boolean;
 	titleBarStyle?: 'native' | 'custom';
+	vscodeWindowId?: string;


@Tyriar I would suggest to call this id and instead fix all users of the current id property to use vscodeWindows.win.id because that makes it very clear what ID is being asked.

bpasero · 2016-12-01T16:24:01Z

+			// TODO: There's an extra empty workspace opening when restoring an empty workspace (sometimes)
+			emptyToRestore.forEach(vscodeWindowId => {
+				const configuration = this.toConfiguration(openConfig);
+				configuration.vscodeWindowId = vscodeWindowId;


@Tyriar why is this done here and not inside the toConfiguration method?

bpasero · 2016-12-01T16:25:57Z

+			configuration.vscodeWindowId = vscodeWindow.vscodeWindowId;
+			if (!configuration.extensionDevelopmentPath) {
+				// TODO: Cover closing a folder/existing window case
+				this.backupService.pushEmptyWorkspaceBackupWindowIdSync(configuration.vscodeWindowId);


@Tyriar I suggest to move this up to the place where we already call pushWorkspaceBackupPathsSync so that we have it in one place. doing this from toConfiguration is very weird. As a caller of this method I would expect to get a configuration object but as a side effect I am writing to the backup folder??

bpasero · 2016-12-01T16:28:11Z

 	pushWorkspaceBackupPathsSync(workspaces: Uri[]): void;
+
+	// TODO: Doc
+	pushEmptyWorkspaceBackupWindowIdSync(vscodeWindowId: string): void;


@Tyriar maybe merge both methods into one and the return type can either be a path to a workspace or a window id? might make the naming easier.

bpasero · 2016-12-01T16:29:03Z


 	execPath: string;
 	appRoot: string;
+	vscodeWindowId: string;


@Tyriar we cannot really add this here because environment service is agnostic to windows, we have one in the shared process as well as the CLI. I suggest to put this into the IWindowService on the renderer side.

bpasero · 2016-12-01T16:38:14Z

@Tyriar do we actually need to come up with and pass around a window id only to know from where to restore backups? Why not let the backup service deal with it:

from the main side you already open as many empty workspaces as you had dirty files in empty workspaces
my backup restorer asks the backup service for a list of backup files to open
the backup service could simply ask the main side: hey give me the contents of one of the empty workspace backup folder
if you have more than one empty workspace, just use the index of the window as it is known to windows.ts to distinguish or keep in memory somewhere that you gave one window a set of backups already

Just a thought...

…ty_workspaces

bpasero · 2016-12-02T06:40:03Z

 		// Backup File Service
 		const workspace = this.contextService.getWorkspace();
-		serviceCollection.set(IBackupFileService, this.instantiationService.createInstance(BackupFileService, workspace ? workspace.resource : null));
+		serviceCollection.set(IBackupFileService, this.instantiationService.createInstance(BackupFileService, this.windowService.getCurrentWindowId()));


@Tyriar why not let backup file service depend on windows service to get the ID?

It could, and it did prior to 85bf25f. Aren't less deps generally a good thing?

bpasero · 2016-12-02T06:43:30Z

+		if (emptyToRestore.length > 0) {
+			emptyToRestore.forEach(backupFolder => {
+				const configuration = this.toConfiguration(openConfig, null, null, null, null, backupFolder);
+				const browserWindow = this.openInBrowserWindow(configuration, openInNewWindow, openInNewWindow ? void 0 : openConfig.windowToUse);


@Tyriar we always want each of these windows to open inside a new window so I suggest to just call

const browserWindow = this.openInBrowserWindow(configuration, true /* new window */);

In the same way as we do for restoring folders.

bpasero · 2016-12-02T06:47:15Z

 			}
 		}

+		this.backupService.registerWindowForBackups(vscodeWindow.id, !configuration.workspacePath, configuration.backupFolder, configuration.workspacePath);


@Tyriar are you missing the check for openConfig.cli.extensionDevelopmentPath on purpose here?

Yes, moved it to registerWindowForBackups so it was handled by the BackupMainService. Now that you mentioned it though, that was there for a good reason, thanks for pointing that out 👍

bpasero · 2016-12-02T06:48:36Z

 	perfWindowLoadTime?: number;

 	workspacePath?: string;
+	backupFolder?: string;


@Tyriar suggest backupPath to keep naming consistent

I removed this specific member in 10c1211

On the naming, folder is used in some placed because it's only folder, as opposed to the full path.

bpasero · 2016-12-02T06:53:45Z

-		if (emptyToOpen.length > 0) {
+		if (emptyToRestore.length > 0) {
+			emptyToRestore.forEach(backupFolder => {
+				const configuration = this.toConfiguration(openConfig, null, null, null, null, backupFolder);


@Tyriar something is weird about backupFolder. As someone that tries to understand the code I would assume every window has an associated backupFolder. but that is not true, only the empty windows to restore have it. I would make this more obvious. Maybe remove the property from IWindowConfiguration and just pass it into openInBrowserWindow method? Also needs a clearer name to make its meaning more obvious.

Done 10c1211

EnvironmentService's config may be stale.

bpasero · 2016-12-02T08:27:20Z

 			emptyToRestore.forEach(emptyWorkspaceBackupFolder => {
 				const configuration = this.toConfiguration(openConfig, null, null, null, null);
-				const browserWindow = this.openInBrowserWindow(configuration, openInNewWindow, openInNewWindow ? void 0 : openConfig.windowToUse, emptyWorkspaceBackupFolder);
+				const browserWindow = this.openInBrowserWindow(configuration, true /* new window */, openInNewWindow ? void 0 : openConfig.windowToUse, emptyWorkspaceBackupFolder);


@Tyriar I would not assume we have a windowToUse in this case ever?

👍 d661321

bpasero · 2016-12-02T08:28:50Z

@@ -51,11 +51,6 @@ export class BackupMainService implements IBackupMainService {
 	}

 	public registerWindowForBackups(windowId: number, isEmptyWorkspace: boolean, backupFolder?: string, workspacePath?: string): void {


@Tyriar I didnt know you had this check in here, I am also fine leaving it there to keep this knowledge inside the service!

That was my thinking, didn't we want to keep that check outside though due to the config in IEnvironmentService potentially being stale or something?

Hm yeah maybe. We can leave it there for now. The problem is that the main side only ever has one environment service and so a second window would not answer this check correctly.

…ty_workspaces

Just in case

Rough initial empty workspace hot exit impl

5517e6f

Part of #13733

Tyriar assigned bpasero Dec 1, 2016

msftclas added the cla-already-signed label Dec 1, 2016

Merge branch 'master' into tyriar/hot_exit/empty_workspaces

2d7b067

bpasero reviewed Dec 1, 2016

View reviewed changes

Tyriar added 10 commits December 1, 2016 18:34

Move all handling of backup workspace path to main, use IPC

df67ad4

Make (empty)workspaceBackupPaths getters on BackupMainService

3903920

Remove workspace and IWindowService dep on BackupFileService

85bf25f

Consolidate registerWindowForBackups calls

8caae43

General clean up

7ba4f89

Remove remnants of vscodeWindowId

3c5f9eb

Reduce duplication in BackupMainService

9a429ba

Remove logs

60b588f

Make a generic BackupMainService.pushBackupPathsSync function

6cb1090

Merge remote-tracking branch 'origin/master' into tyriar/hot_exit/emp…

5879e71

…ty_workspaces

bpasero reviewed Dec 2, 2016

View reviewed changes

Tyriar added 3 commits December 1, 2016 23:17

Make backupFolder a openInBrowserWindow param, improve name

10c1211

Make empty workspace windows always open as new window

d979589

Move register backups extension dev check back to windows

a3e67ee

EnvironmentService's config may be stale.

bpasero reviewed Dec 2, 2016

View reviewed changes

Tyriar added 7 commits December 2, 2016 00:38

Never reuse a window for empty workspaces being restored

d661321

Merge remote-tracking branch 'origin/master' into tyriar/hot_exit/emp…

9ead89a

…ty_workspaces

Fix tests

b158bfb

BackupMainService.getBackupPath tests

bd06b69

Add loadSync for empty workspaces test coverage

c3cea7a

Clean up BackupMainService tests into suites

2a17b5c

Add another getWorkspaceHash test

8ba5d11

Tyriar added 7 commits December 2, 2016 15:49

Merge remove workspace funcs to removeBackupPathSync

6aec1e7

Add test for empty workspace removal

d16fb34

Tidy up BackupFileService test

3c6821e

Merge remote-tracking branch 'origin/master' into tyriar/hot_exit/emp…

2c2ee3a

…ty_workspaces

Remove redundant call

65c4f73

Merge duplicate workspace keys on windows/mac

eb9a79d

Just in case

Fix tests

ef4b510

Tyriar merged commit d5349d2 into master Dec 3, 2016

Tyriar deleted the tyriar/hot_exit/empty_workspaces branch December 3, 2016 04:13

github-actions Bot locked and limited conversation to collaborators Mar 27, 2020

		@@ -51,11 +51,6 @@ export class BackupMainService implements IBackupMainService {
		}

		public registerWindowForBackups(windowId: number, isEmptyWorkspace: boolean, backupFolder?: string, workspacePath?: string): void {

Conversation

Tyriar commented Dec 1, 2016

Uh oh!

Tyriar commented Dec 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bpasero commented Dec 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Tyriar commented Dec 1, 2016 •

edited

Loading