rock-core / vscode-rock

VSCode extension for Rock integration
MIT License
1 stars 1 forks source link

Improve multi workspace handling #62

Closed doudou closed 5 years ago

doudou commented 5 years ago

This pull request improves having multiple Autoproj workspaces in a single VSCode workspace. In addition, it workarounds an issue related to VSCode's handling of the very first folder in the workspace.

First the vscode issue. VSCode will restart the extension hosts if the toplevel folder in a VSCode workspace changes (because it needs to change the now-obsolete rootPath property). This leads to the extension attempting to start a Watch while there is already one. The first attempt was to make sure we deregister the watch task using the new Task execution APIs. This is a great addition in the first place, but proved fruitless as it seems that vscode doesn't shut down the extensions cleanly (I wouldn't get the extension's deactivate function or any of the dispose methods to be called). I then tried to use the taskExecutions property to find the watch task, but it is also empty - don't really know why.

In fine, I tried to see a way to make sure the first folder doesn't change this much. This led me to think about a problem I recently had, to use two autoproj workspaces in the same vscode workspace (comparing code). This was hell, as the packages would be named the same.

So, I went for:

Because the folder names can't be changed after they have been added, I also created a Rock: Add Workspace command for the initial add. The one corner case is if someone adds the autoproj folder directly.

Illustration:

vscode-multi-workspace

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 70


Changes Missing Coverage Covered Lines Changed/Added Lines %
src/config.ts 0 1 0.0%
src/context.ts 1 2 50.0%
src/wrappers.ts 0 4 0.0%
src/autoproj.ts 2 7 28.57%
src/extension.ts 0 12 0.0%
src/commands.ts 35 48 72.92%
src/vscode_workspace_manager.ts 32 55 58.18%
<!-- Total: 70 129 54.26% -->
Files with Coverage Reduction New Missed Lines %
src/extension.ts 6 0.0%
<!-- Total: 6 -->
Totals Coverage Status
Change from base Build 65: 0.08%
Covered Lines: 1010
Relevant Lines: 1258

💛 - Coveralls
g-arjones commented 5 years ago

This leads to the extension attempting to start a Watch while there is already one

I handled this by:

  1. Keeping track of "Watch" tasks
  2. Killing watch tasks when the extension gets disposed

Which seems to work fine, AFAICT.

Because the folder names can't be changed after they have been added

I don't think this is correct... You can rename the folder by doing:

vscode.workspace.updateWorkspaceFolders(0, index, { name: 'foo', uri: folderUri });

See this

Still, I like the idea of having the main configuration automatically added when a new pacakage is added (and removed when all packages from the workspace get removed)

doudou commented 5 years ago

(and removed when all packages from the workspace get removed)

Actually, that's something I really don't want to do.

doudou commented 5 years ago
2\. Killing watch tasks when the extension gets disposed

Well, then, I don't know why, but according to the tests I did yesterday, the extension doesn't get disposed in the case where the first workspace folder is changed. The PR implements the same task management behavior - but through tasks instead of relying on the PID file - and I still get the same errors. It appeared that the dispose function was not called.

g-arjones commented 5 years ago

but through tasks instead of relying on the PID file

Not file, just a Map() from workspacePath to pid.

dispose() does get called but you can't do anything async in it (i.e use vscode's own terminateTask api) because apparently the extension host won't get the chance to execute that

doudou commented 5 years ago

dispose() does get called but you can't do anything async in it (i.e use vscode's own terminateTask api) because apparently the extension host won't get the chance to execute that

That would explain.

doudou commented 5 years ago

dispose() does get called but you can't do anything async in it (i.e use vscode's own terminateTask api) because apparently the extension host won't get the chance to execute that

terminate() is not even async ... they really are not letting the extensions do any form of cleanup.

doudou commented 5 years ago

Anyways:

g-arjones commented 5 years ago

Looks good to me.. I liked the idea of creating one disposable per watch task. Cool stuff... 👍

doudou commented 5 years ago

I liked the idea of creating one disposable per watch task.

After working with vscode, I "stole" the disposable pattern in Roby itself. It's a really neat pattern.