Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make DebugSession.name writable; fixes #79583 #80122

Merged
merged 1 commit into from Sep 5, 2019

Conversation

@dgozman
Copy link
Contributor

commented Aug 30, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2019

@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 Outdated Show resolved Hide resolved
@@ -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;

This comment has been minimized.

Copy link
@isidorn

isidorn Sep 2, 2019

Contributor

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.

This comment has been minimized.

Copy link
@isidorn

isidorn Sep 2, 2019

Contributor

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.

This comment has been minimized.

Copy link
@weinand

weinand Sep 3, 2019

Member

Yes, that's the approach we should use.

This comment has been minimized.

Copy link
@dgozman

dgozman Sep 3, 2019

Author Contributor

Sounds good, updated.

@isidorn isidorn added this to the September 2019 milestone Sep 2, 2019
@weinand weinand self-requested a review Sep 3, 2019
@isidorn

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

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 Outdated Show resolved Hide resolved
Copy link
Member

left a 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.

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

@isidorn

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

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?

@dgozman dgozman force-pushed the dgozman:fix-79583 branch from 4268d35 to e5e02ba Sep 3, 2019
@isidorn

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

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

Copy link
Contributor Author

left a comment

Please take another look.

src/vs/vscode.d.ts Outdated Show resolved Hide resolved
src/vs/workbench/api/node/extHostDebugService.ts Outdated Show resolved Hide resolved
@@ -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;

This comment has been minimized.

Copy link
@dgozman

dgozman Sep 3, 2019

Author Contributor

Sounds good, updated.

@isidorn

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

@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 dgozman force-pushed the dgozman:fix-79583 branch from e5e02ba to cb6b17d Sep 4, 2019
Copy link
Contributor Author

left a comment

@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.

@isidorn

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

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!

@isidorn isidorn merged commit 97a05ce into microsoft:master Sep 5, 2019
2 checks passed
2 checks passed
VS Code #20190904.106 succeeded
Details
license/cla All CLA requirements met.
Details
@dgozman

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

Thank you, I will look at #80379.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.