Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Conversation

@rchiodo
Copy link

@rchiodo rchiodo commented Jan 22, 2021

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.

return new Position(0, 0);
}

private raiseCellInsertion(newUris: Uri[], oldUris: Uri[]) {

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) {

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)) {

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

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.

Copy link
Author

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).

Copy link

@DonJayamanne DonJayamanne left a 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

@rchiodo rchiodo requested a review from DonJayamanne January 25, 2021 18:47
@rchiodo
Copy link
Author

rchiodo commented Jan 25, 2021

@DonJayamanne did you have any more comments?

this.onCellsChangedEmitter.fire({
document: this,
contentChanges: [
{

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

Copy link
Author

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: [

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.

Copy link

@DonJayamanne DonJayamanne left a 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-io
Copy link

Codecov Report

Merging #15208 (8c7cbc1) into main (b0368ef) will decrease coverage by 0%.
The diff coverage is 3%.

@@           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     
Impacted Files Coverage Δ
...client/jupyter/languageserver/notebookConverter.ts 1% <0%> (-1%) ⬇️
.../jupyter/languageserver/notebookMiddlewareAddon.ts 3% <0%> (-1%) ⬇️
...t/jupyter/languageserver/notebookConcatDocument.ts 5% <3%> (-3%) ⬇️
src/client/common/process/pythonEnvironment.ts 81% <0%> (-9%) ⬇️
src/client/interpreter/display/progressDisplay.ts 83% <0%> (-4%) ⬇️
src/client/tensorBoard/helpers.ts 73% <0%> (-4%) ⬇️
src/client/common/installer/productInstaller.ts 40% <0%> (-4%) ⬇️
src/client/pythonEnvironments/legacyIOC.ts 31% <0%> (-4%) ⬇️
...rc/client/common/process/pythonExecutionFactory.ts 87% <0%> (-1%) ⬇️
...client/interpreter/virtualEnvs/virtualEnvPrompt.ts 78% <0%> (-1%) ⬇️
... and 32 more

@rchiodo rchiodo merged commit ae6abc0 into microsoft:main Jan 25, 2021
@rchiodo rchiodo deleted the rchiodo/cell_newlines branch January 25, 2021 23:09
const timer = setTimeout(() => {
reject(new Error("Command 'workbench.action.closeAllEditors' timed out"));
}, 15000);
}, 2000);
Copy link

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?

Copy link
Author

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.

Copy link
Author

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;
        }
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog No news entry required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.