Support moving/adding/deleting cells in notebook intellisense#15208
Conversation
| return new Position(0, 0); | ||
| } | ||
|
|
||
| private raiseCellInsertion(newUris: Uri[], oldUris: Uri[]) { |
There was a problem hiding this comment.
This API supports more than one cell being inserted, but we're only looking for & firing a single event for one of the cells.
I think we should fire events for each new cell.
| private raiseCellDeletion(newUris: Uri[], oldUris: Uri[]) { | ||
| // A cell was deleted. Figure out which one | ||
| const index = oldUris.findIndex((p) => !newUris.includes(p)); | ||
| if (index >= 0) { |
There was a problem hiding this comment.
Same as previous comment, fire multiple events, one for each deleted cell.
| this.raiseCellInsertion(newUris, oldUris); | ||
| } else if (this.cellTracking.length > this.notebook.cells.length) { | ||
| this.raiseCellDeletion(newUris, oldUris); | ||
| } else if (!isEqual(oldUris, newUris)) { |
There was a problem hiding this comment.
Can we confirm isEqual works with class instances.
I.e. assume we have two instances of the Uri class both of which point to A, will isEqual return true.
Based on my tests it doesn't, in which casee we might want to toString when omparing
There was a problem hiding this comment.
Or use the Api arePathsEqual or similar, which accepts Uris.
There was a problem hiding this comment.
This is definitely working. I'm hesitant to make this more complicated. I'll try toString(). Are paths equal doesn't include the cell indicator (just the document path).
DonJayamanne
left a comment
There was a problem hiding this comment.
Need to fire events for multiple cells & might have to tweek usage of isEqual
|
@DonJayamanne did you have any more comments? |
| this.onCellsChangedEmitter.fire({ | ||
| document: this, | ||
| contentChanges: [ | ||
| { |
There was a problem hiding this comment.
I htink we should batch the changes into a single event here. I.e. contentChanges should be an array of the changes, else defeats the purpose of having the array contentChanges
| // Turn this cell into a change event. | ||
| this.onCellsChangedEmitter.fire({ | ||
| document: this, | ||
| contentChanges: [ |
There was a problem hiding this comment.
Same here, single event with multiple entries.
Apologies for the prvious comment, i didn't realize this event supported triggering a single event with all changes.
DonJayamanne
left a comment
There was a problem hiding this comment.
Please can we fire a single event with all the changes in contentChanges array.
I think that'll be better use of the existing api instead of triggering multiple events.
Codecov Report
@@ Coverage Diff @@
## main #15208 +/- ##
=======================================
- Coverage 65% 64% -1%
=======================================
Files 561 559 -2
Lines 26560 26683 +123
Branches 3814 3854 +40
=======================================
+ Hits 17292 17337 +45
- Misses 8562 8639 +77
- Partials 706 707 +1
|
| const timer = setTimeout(() => { | ||
| reject(new Error("Command 'workbench.action.closeAllEditors' timed out")); | ||
| }, 15000); | ||
| }, 2000); |
There was a problem hiding this comment.
@rchiodo It seems we have flaky CI failures because of this "Command 'workbench.action.closeAllEditors' timed out": https://github.com/microsoft/vscode-python/runs/1825248387?check_suite_focus=true
Why was this change made?
There was a problem hiding this comment.
Because the command was causing other spots to timeout.
On our side we actually removed the timeout altogether. If close all editors doesn't work, then whatever. It doesn't really affect the test.
There was a problem hiding this comment.
This is what our closeWindowsInternal looks like now:
async function closeWindowsInternal() {
// If there are no editors, we can skip. This seems to time out if no editors visible.
if (
!vscode.window.visibleTextEditors ||
(vscode.env.appName.toLowerCase().includes('insiders') && !vscode.window.visibleNotebookEditors)
) {
// Instead just post the command
void vscode.commands.executeCommand('workbench.action.closeAllEditors');
return;
}
class CloseEditorsTimeoutError extends Error {
constructor() {
super("Command 'workbench.action.closeAllEditors' timed out");
}
}
const closeWindowsImplementation = (timeout = 2_000) => {
return new Promise<void>((resolve, reject) => {
// Attempt to fix #1301.
// Lets not waste too much time.
const timer = setTimeout(() => reject(new CloseEditorsTimeoutError()), timeout);
vscode.commands.executeCommand('workbench.action.closeAllEditors').then(
() => {
clearTimeout(timer);
resolve();
},
(ex) => {
clearTimeout(timer);
reject(ex);
}
);
});
};
// For some reason some times the command times out.
// If this happens, just wait & retry, no idea why VS Code is flaky.
// Lets wait & retry executing the command again, hopefully it'll work second time.
try {
await closeWindowsImplementation();
} catch (ex) {
if (ex instanceof CloseEditorsTimeoutError) {
// Do nothing. Just stop waiting.
} else {
throw ex;
}
}
}
For #microsoft/vscode-jupyter#4300
The concat of a notebook into a single python file works fine if the user edits cells. However when adding new cells, no changes were being made to the file. Nor when deleting or moving cells.
This change introduces change events when deleting/adding/moving cells as if the user typed the new cells out.