Upgrade to new lsp middleware concat #8219
Conversation
| // Stick in our list of associated documents and indicate controller may have changed. | ||
| if (!this.associatedDocuments.has(notebook)) { | ||
| this.associatedDocuments.add(notebook); | ||
| this._onNotebookControllerSelected.fire({ notebook, controller: this }); |
There was a problem hiding this comment.
Setting affinity is not the same as setting it as selected.
You could have a notebook that's associated with kernel A, but the recommended kernel (affinity) is Kernel B.
There was a problem hiding this comment.
I'd consider raising a seprate event and or having a seprate method to get the recommended kernel.
I believe you need this for instances where a kernel isn't seleced, but want to provide the right intellisense, if that's the case a seprate property/event would be better.
There was a problem hiding this comment.
No this is exactly what I want.
This is what is happening now.
- User opens a notebook, kernel is associated with document, notebook selection change event fires
- User closes notebook
- User reopens notebook. Nothing fires, controller doesn't even know what the selection is.
I think what should happen is the same thing as the first one
- User opens a notebook, kernel is associated with document, notebook selection change event fires
There was a problem hiding this comment.
I need this so that on second open, the intellisense is using the correct (selected) kernel.
There was a problem hiding this comment.
On first open, the reason it works is because the selection change event fires after setting affinity.
There was a problem hiding this comment.
This code here:
Which is triggered by this:
On a second open that selection changed event doesn't fire.
There was a problem hiding this comment.
Ok got it, but if you create a brand new notebook, then this event will still get fired even when no kernel is selected.
Today there's a bug in VS Code causing kernels to get auto selecfed, hence this will work.
I'll merge my fix tomorrow into VS Code and then we can re-visit this PR.
I still don't think this is right.
There was a problem hiding this comment.
Okay trying a different way. We can just remember it like VS code does. When it gets set, stick it in a map.
| // Stick in our list of associated documents and indicate controller may have changed. | ||
| if (!this.associatedDocuments.has(notebook)) { | ||
| this.associatedDocuments.add(notebook); | ||
| this._onNotebookControllerSelected.fire({ notebook, controller: this }); |
There was a problem hiding this comment.
I'd consider raising a seprate event and or having a seprate method to get the recommended kernel.
I believe you need this for instances where a kernel isn't seleced, but want to provide the right intellisense, if that's the case a seprate property/event would be better.
Codecov Report
@@ Coverage Diff @@
## main #8219 +/- ##
=======================================
- Coverage 71% 58% -14%
=======================================
Files 365 368 +3
Lines 22573 22589 +16
Branches 3413 3419 +6
=======================================
- Hits 16182 13184 -2998
- Misses 4997 8240 +3243
+ Partials 1394 1165 -229
|
For #8075
Need to do more testing to make sure this all works. Found a problem where the notebook controller wasn't being indicated on reopen of a notebook.
Also had to wire up the 'refresh' for the concat document (when a notebook cells move around)