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

added the workspace symbol provider for markdown #46406 #47610

Merged

Conversation

@pradeepmurugesan
Copy link
Contributor

commented Apr 10, 2018

@mjbvz
Copy link
Contributor

left a comment

Thanks for looking into this! Just a few changes before we can merge this in. Let me know if you have any questions about my comments


public constructor(symbolProvider: MDDocumentSymbolProvider) {
this.symbolProvider = symbolProvider;
this.populateSymbolCache();

This comment has been minimized.

Copy link
@mjbvz

mjbvz Apr 10, 2018

Contributor

This should be done lazily when workspace symbols are first requested

This comment has been minimized.

Copy link
@pradeepmurugesan

pradeepmurugesan Apr 11, 2018

Author Contributor

done..

}

private registerOnSaveEvent(): void {
workspace.onDidSaveTextDocument(async document => {

This comment has been minimized.

Copy link
@mjbvz

mjbvz Apr 10, 2018

Contributor

This registration returns a registration handle that handles unregistering the event handler. Add a dispose method on MarkdownWorkspaceSymbolProvider and then call .dipose() on the the registration handle to make sure it is properly cleaned up

This comment has been minimized.

Copy link
@pradeepmurugesan

pradeepmurugesan Apr 11, 2018

Author Contributor

@mjbvz I have one question here. I have made the changes though.. Excuse me if my question is naive..

Who calls the dispose method? I couldn't find the same in the WorkspaceSymbolProvider interface as well.. I assume there should be a common place to check if the method "dispose" exists on an object and then invoke this.

Can you kindly point me to that.

@mjbvz mjbvz self-assigned this Apr 10, 2018

@duttaditya18
Copy link

left a comment

Awesome!

@pradeepmurugesan

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2018

@mjbvz got a chance to look into the changes.. ?

@mjbvz mjbvz added this to the April 2018 milestone Apr 13, 2018

@mjbvz mjbvz merged commit 5e993f7 into microsoft:master Apr 13, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
@mjbvz

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2018

Thanks! Will be in the next insiders build and in VS Code 1.23

@pradeepmurugesan

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2018

@mjbvz Thanks for merging.. I had asked a question in the review comment. It will be really helpful if you could answer the same.

Who calls the dispose method? I couldn't find the same in the WorkspaceSymbolProvider interface as well.. I assume there should be a common place to check if the method "dispose" exists on an object and then invoke this.

Can you kindly point me to that.

@mjbvz

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2018

In this case it will never get called. The extension would have to register the provider as one of its disposables but this is pointless because the extension itself never gets disposed. It's still good practice to clean up any resources in a dispose method though, which does come in handy today when writing tests

@mjbvz

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2018

@pradeepmurugesan By the way, do you have a twitter handle that you would like mentioned when we call out this feature? Tweeting can help get some additional testing on the feature and I want to make sure proper credit is given for your work

@pradeepmurugesan

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2018

@mjbvz Sure.. This is my twitter profile https://twitter.com/pradeepm_14. Handle- @pradeepm_14

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.