-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Support moving/adding/deleting cells in notebook intellisense #15208
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
Conversation
| return new Position(0, 0); | ||
| } | ||
|
|
||
| private raiseCellInsertion(newUris: Uri[], oldUris: Uri[]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or use the Api arePathsEqual or similar, which accepts Uris.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do
| // Turn this cell into a change event. | ||
| this.onCellsChangedEmitter.fire({ | ||
| document: this, | ||
| contentChanges: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.