search

davraamides / todotxt-mode

MIT License
59 stars 12 forks source link

Unregister hover providers when unloaded #43

Closed willburden closed 2 years ago

willburden commented 2 years ago

This is just a tiny fix for a bug I was having (#42), if you want it. It isn't at all hacky - I've found multiple examples online that suggest this is a standard thing you're supposed to do when registering hover providers, so it really should be there.

Tested it on my computer where I was having the bug, and it seems completely fixed!

Screenshot from 2021-11-16 19-14-02

willburden commented 2 years ago

I've tried to make it ready to be published, so you could just clone it and run vsce publish patch. The changelog explains version 1.4.30 and the package.json still has it as v1.4.29, so when vsce increments the version number everything should be good.

davraamides commented 2 years ago

Nice catch, @willburden! I reviewed your PR but just had one question: do I need to do any cleanup on the context.subscriptions array? The docs for the subscriptions say "When this extension is deactivated the disposables will be disposed" but should I be doing something to dispose them earlier? What if the extension is never deactivated?

willburden commented 2 years ago

I'm not an expert on vscode but here's my understanding. From the doc for registerHoverProvider:

Returns - A Disposable that unregisters this provider when being disposed.

So when I push that disposable to subscriptions I'm just telling vscode to unregister the hover provider when the extension is unloaded. Take a look at this section in the guide - it's exactly the same but with a command instead of a hover provider.

If the extension is never deactivated/unloaded, the hover provider will stay registered for the entire session - which is exactly you want! Similarly, if you disposed of the disposables early then that would unregister the hover provider while the extension is still supposed to be active, disabling the hover behaviour when you don't want it to.

Does that make sense and seem correct?

davraamides commented 2 years ago

I guess. I just thought the bug you had found was that the hover providers weren't being disposed and that if you just put them in a collection, they still won't until the collection is disposed. But if your change fixed the issue, then that's good enough for me. I'll apply the changes and push out a new version tonight or tomorrow.

willburden commented 2 years ago

Yeah but it's a special collection that vscode automatically disposes when the extension is unloaded :)