Skip to content
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

Notebook cell not saving its contents #135516

Closed
roblourens opened this issue Oct 20, 2021 · 6 comments
Closed

Notebook cell not saving its contents #135516

roblourens opened this issue Oct 20, 2021 · 6 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug important Issue identified as high-priority insiders-released Patch has been released in VS Code Insiders notebook-serialization verified Verification succeeded
Milestone

Comments

@roblourens
Copy link
Member

@greazer has a repro currently that I will try to debug more tomorrow

  • Edit one particular cell in a github-issues notebook, save
  • The saved content is not written to the json file
  • Works for other cells

Questions to answer

  • Does the dirty indicator show up for that cell?
  • Did we invoke the notebook serializer?
  • Is the text in the textmodel on the frontend (probably) and the extension host?
@roblourens roblourens added bug Issue identified by VS Code Team member as probable bug important Issue identified as high-priority notebook-serialization labels Oct 20, 2021
@roblourens roblourens added this to the October 2021 milestone Oct 20, 2021
@rebornix
Copy link
Member

rebornix commented Oct 21, 2021

Reproduce steps:

Open two VS Code instances with same folder (Stable + Insiders)

  • Instance A
    • Open notebook_1
  • Instance B
    • Open notebook_1
    • Modify cell_1
    • Save
  • Instance A
    • Make sure cell_1 is refreshed
    • Update cell_2
    • cell_2 is now never saved to disk

It seems that the auto reload on file change on disk triggers this, the UI is attached to a new text buffer/model, however the saving/snapshot code is using another one.

@jrieken
Copy link
Member

jrieken commented Oct 22, 2021

Slightly confused by those steps. Do you mean the same notebook in both instances and not notebook_1 and notebook_2? Also, should the 3. step really happen in instance B and back in instance A?

edit fixed the steps above because that's how I could reproduce

@jrieken
Copy link
Member

jrieken commented Oct 22, 2021

This is a problem with the NotebookCellTextModel. It uses two competing data sources: TextBuffer and TextModel and in this case the TextModel is correctly updated but the TextBuffer isn't. The bug occurs because for saving we use the textual value of the latter

Screen Shot 2021-10-22 at 12 38 23

Having these two data sources doesn't seem healthy and IMO the presence of a resolved text model should always shadow the text buffer so that there is only one source of truth

@rebornix
Copy link
Member

@jrieken when we create the text model, we actually use the text buffer from the cell as the source of truth

const bufferFactory: ITextBufferFactory = {
create: (defaultEOL) => {
const newEOL = (defaultEOL === DefaultEndOfLine.CRLF ? '\r\n' : '\n');
(cell.textBuffer as ITextBuffer).setEOL(newEOL);
return { textBuffer: cell.textBuffer as ITextBuffer, disposable: Disposable.None };
},

not sure how the disconnect of textmodel <-> textbuffer happens. I'll dig into this a bit more.

@rebornix rebornix self-assigned this Oct 25, 2021
@rebornix
Copy link
Member

When we reload the window, we would create a notebook text model with new cell text models, and try to link alive monaco text models back to the cell models, but we didn't reuse the text buffer from them

if (textModel && textModel instanceof TextModel) {
cell.textModel = textModel;

@rebornix
Copy link
Member

rebornix commented Oct 25, 2021

Glad we nailed it before release, good catch and thanks all for the help.

@IanMatthewHuff IanMatthewHuff added verified Verification succeeded and removed verified Verification succeeded labels Oct 27, 2021
@tanhakabir tanhakabir added verified Verification succeeded and removed verified Verification succeeded labels Oct 28, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Dec 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug important Issue identified as high-priority insiders-released Patch has been released in VS Code Insiders notebook-serialization verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

8 participants
@roblourens @IanMatthewHuff @rebornix @bpasero @jrieken @DonJayamanne @tanhakabir and others