Make DebugSession.name writable; fixes #79583 #80122

dgozman · 2019-08-30T16:32:35Z

@isidorn I took a stub on issue #79583, could you please take a look? Do you think this is a right direction?

I thought about constraining the name setter only to the extension which provides the debugger type for this DebugSession, but wasn't able to get a hold on extension to implement this check. Any thoughts?

This is changing extension API, so perhaps this process applies? I am not sure how I can follow up there.

Here is a branch I used for testing, it defines "Rename active session" command.

isidorn · 2019-09-02T17:26:10Z

@dgozman thanks for this PR, I think this direction makes sense but I will also consult with @weinand tomorrow.

As for the API, this would be a minor API update, so I think we can keep the process about adding new API to the minimum and I can take care of that.

Regarding the actuall code changes I have left comments in the code directly. Thanks a lot!

src/vs/workbench/api/node/extHostDebugService.ts

isidorn · 2019-09-02T17:29:25Z

src/vs/workbench/contrib/debug/browser/debugSession.ts

@@ -108,6 +110,11 @@ export class DebugSession implements IDebugSession {
 		return includeRoot && this.root ? `${this.configuration.name} (${resources.basenameOrAuthority(this.root.uri)})` : this.configuration.name;
 	}

+	setName(name: string): void {
+		this.configuration.name = name;


I do not think we should change the underlying configuration object.
This to me feels like we should introduce a new attribute on the session called name.
And in the getLabel we use the name of the session, if the name is not defined then go back to providing the configuration.name as a fallback.

That way we have two independt attributes, and we do not mix concepts between the name of the configuration and the name of the session.
Let me know what you think.

Yes, that's the approach we should use.

Sounds good, updated.

src/vs/workbench/contrib/debug/browser/loadedScriptsView.ts

isidorn · 2019-09-03T10:49:15Z

For the API I have created this feature request #80257
We will discuss this in todays API sync meeting and I will provide the update here.

src/vs/vscode.d.ts

weinand

What is missing from the PR is the change propagation from VS Code into an extension. Use case: one extension updates a session's name. This flows nicely back into VS Code and updates the UI but the name change is not propagated to debug session copies that might live in other extensions.

To make this API complete, we would actually need listener API for tracking the name changes of debug sessions.

isidorn · 2019-09-03T15:53:46Z

After discussing this in the API sync we agreed:

This approach is good.
We can add the name change event later if it is needed
We should not worry about other extensions being able to change the name (there are worse things since they can send customRequests already)

@dgozman so it would be great if you tackle my comments in the code and we can merge this in. Thanks a lot

dgozman · 2019-09-03T15:54:20Z

Thank you for looking at this, I will address all comments. One question on the last comment.

What is missing from the PR is the change propagation from VS Code into an extension. Use case: one extension updates a session's name. This flows nicely back into VS Code and updates the UI but the name change is not propagated to debug session copies that might live in other extensions.

I wanted to do that, but wasn't sure about objects relationship. It seems that MainThreadDebugService has a single _proxy: ExtHostDebugServiceShape to notify about changes, so I didn't see a way to broadcast to multiple ones. Now after your comment, I suspect there are multiple MainThreadDebugService instances so that each of them should update?

isidorn · 2019-09-03T16:17:55Z

@dgozman after discussions @weinand and me decided to not expose the change event. So that will not be needed. Thanks a lot.

dgozman

Please take another look.

src/vs/vscode.d.ts

src/vs/workbench/api/node/extHostDebugService.ts

dgozman · 2019-09-03T16:08:40Z

src/vs/workbench/contrib/debug/browser/debugSession.ts

@@ -108,6 +110,11 @@ export class DebugSession implements IDebugSession {
 		return includeRoot && this.root ? `${this.configuration.name} (${resources.basenameOrAuthority(this.root.uri)})` : this.configuration.name;
 	}

+	setName(name: string): void {
+		this.configuration.name = name;


Sounds good, updated.

src/vs/workbench/api/browser/mainThreadDebugService.ts

src/vs/workbench/contrib/debug/browser/callStackView.ts

src/vs/workbench/contrib/debug/browser/debugActionViewItems.ts

src/vs/workbench/contrib/debug/browser/debugSession.ts

isidorn · 2019-09-04T09:17:59Z

@dgozman thanks a lot for addressing my initial feedback.
I have done an additional review, can you check out my comments and try to adress those if they make sense to you?

Great for the testing branch. Once you wrap up the changes can you test everything again just so we are sure all is good. I will also test it before merging. Things to keep an eye on when testing:

Call Stack View gets properly updated
Debug Toolbar dropdown gets updated
Debug Console session dropdown is also up to date
Other extensions when accessing the debug sessions always get the name which is up to date. So a great test would be to have one extensions which changes the name every n seconds, and another extensions gets the session and writes the name. Just so we are sure the data nicely flows through

dgozman

@isidorn Thanks, I have addressed the feedback. All the cases you mentioned work for me locally, including the two extensions case. I also checked the loaded sources view, and it does update as well.

src/vs/workbench/api/browser/mainThreadDebugService.ts

src/vs/workbench/contrib/debug/browser/callStackView.ts

src/vs/workbench/contrib/debug/browser/debugActionViewItems.ts

src/vs/workbench/contrib/debug/browser/debugSession.ts

isidorn · 2019-09-05T09:30:23Z

This works great, I have tested this out and I have found only one issue which I have captured here #80379

Thanks a lot for this PR, nice work!

dgozman · 2019-09-05T16:50:59Z

Thank you, I will look at #80379.

roblourens assigned isidorn and weinand Aug 30, 2019

isidorn requested changes Sep 2, 2019

View reviewed changes

isidorn added this to the September 2019 milestone Sep 2, 2019

weinand self-requested a review September 3, 2019 10:45

isidorn mentioned this pull request Sep 3, 2019

Introduce API to change DebugSession.name #80257

Closed

weinand reviewed Sep 3, 2019

View reviewed changes

src/vs/vscode.d.ts Outdated Show resolved Hide resolved

weinand reviewed Sep 3, 2019

View reviewed changes

dgozman force-pushed the fix-79583 branch from 4268d35 to e5e02ba Compare September 3, 2019 16:14

dgozman commented Sep 3, 2019

View reviewed changes

isidorn requested changes Sep 4, 2019

View reviewed changes

Make DebugSession.name writable; fixes microsoft#79583

cb6b17d

dgozman force-pushed the fix-79583 branch from e5e02ba to cb6b17d Compare September 4, 2019 22:06

dgozman commented Sep 4, 2019

View reviewed changes

isidorn merged commit 97a05ce into microsoft:master Sep 5, 2019

anthonydresser mentioned this pull request Sep 13, 2019

Merge from vscode 64980ea1f3f532c82bb6c28d27bba9ef2c5b4463 microsoft/azuredatastudio#7206

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make DebugSession.name writable; fixes #79583 #80122

Make DebugSession.name writable; fixes #79583 #80122

dgozman commented Aug 30, 2019

isidorn commented Sep 2, 2019

isidorn Sep 2, 2019

isidorn Sep 2, 2019

weinand Sep 3, 2019

dgozman Sep 3, 2019

isidorn commented Sep 3, 2019

weinand left a comment •

edited

Loading

isidorn commented Sep 3, 2019

dgozman commented Sep 3, 2019

isidorn commented Sep 3, 2019 •

edited

Loading

dgozman left a comment

dgozman Sep 3, 2019

isidorn commented Sep 4, 2019

dgozman left a comment

isidorn commented Sep 5, 2019

dgozman commented Sep 5, 2019

Make DebugSession.name writable; fixes #79583 #80122

Make DebugSession.name writable; fixes #79583 #80122

Conversation

dgozman commented Aug 30, 2019

isidorn commented Sep 2, 2019

isidorn Sep 2, 2019

Choose a reason for hiding this comment

isidorn Sep 2, 2019

Choose a reason for hiding this comment

weinand Sep 3, 2019

Choose a reason for hiding this comment

dgozman Sep 3, 2019

Choose a reason for hiding this comment

isidorn commented Sep 3, 2019

weinand left a comment • edited Loading

Choose a reason for hiding this comment

isidorn commented Sep 3, 2019

dgozman commented Sep 3, 2019

isidorn commented Sep 3, 2019 • edited Loading

dgozman left a comment

Choose a reason for hiding this comment

dgozman Sep 3, 2019

Choose a reason for hiding this comment

isidorn commented Sep 4, 2019

dgozman left a comment

Choose a reason for hiding this comment

isidorn commented Sep 5, 2019

dgozman commented Sep 5, 2019

weinand left a comment •

edited

Loading

isidorn commented Sep 3, 2019 •

edited

Loading