microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
164.19k stars 29.29k forks source link

Using applyEdit with replaceCellMetadata or replaceCellOutput fails with no output #105624

Closed colombod closed 4 years ago

colombod commented 4 years ago

@jrieken @roblourens

using the api

         const cellIndex = document.cells.findIndex(c => c === cell);
         if (cellIndex >= 0) {
             const edit = new vscode.WorkspaceEdit();
             edit.replaceCellMetadata(document.uri, cellIndex, metadata);
             vscode.workspace.applyEdit(edit);
         }

doesn't update after first usage

jrieken commented 4 years ago

There were still quite some issues with this on Friday, so trying today's insiders might show other results. Here are a bunch of tests that seem to be green.

doesn't update after first usage

This rings a bell. You aren't awaiting the applyEdit-call which might be the problem here after all

jrieken commented 4 years ago

Seeing an error because transformEditsOutputs is missing. Pushing a fix for that but sometimes there is also errors like this

Error: Notebook 'file:///Users/jrieken/Code/_samples/devfest/103594.github-issues' has changed in the meantime
    at BulkCellEdits.apply (bulkCellEdits.js:43)
    at async BulkEdit._performCellEdits (bulkEditService.js:86)
    at async BulkEdit.perform (bulkEditService.js:65)
    at async BulkEditService.apply (bulkEditService.js:132)

That's a bad sign because it means we don't propagate all changes from the renderer to extension host side...

jrieken commented 4 years ago

Ok, another source of trouble is the version id of the notebook. We send the version id that we know on the extension host and compare that with renderer's value of the version id. If they are different, we discard the edit. However, the are quite some places where the version id changes (in the renderer) without us sending a message to the extension host, e.g when cell text changes the notebook's version id increases but that's not forwarded to the extension host.

This needs some thinking and is part of larger clean-up work we need to do in notebook's land (fyi @rebornix). For now, I will disable the version check and optimistically accept all edits from the extension host

colombod commented 4 years ago

Is there going to be a vscode update then that will allow me to merge this PR or should we call it off for the moment?

jrieken commented 4 years ago

Should be in next/tomorrow's insiders... We are currently winding down for the August endgame, so until mid/end of week we will have new insiders builds and then we'll freeze insiders in preparation for the release