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 output optimization #14234

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

jonah-iden
Copy link
Contributor

What it does

Changes the notebook output rendering to only use a single webview for each editor.
This should also fix some previously existent issues like with moving cells having outputs and changing mime type.

How to test

Open a notebook and test that everything regarding rendering of outputs still works.

  • opening notebooks with outputs
  • executing cells
  • moving/deleting cells with outputs
  • creating new cells infornt/in between cells with outputs
  • collapsing outputs
  • clearing outputs
  • chainging output presentation type

Follow-ups

Review checklist

Reminder for reviewers

@jonah-iden jonah-iden force-pushed the jiden/notebook-output-optimization branch from 76ea74b to 10a5c34 Compare September 30, 2024 15:13
@msujew msujew added notebook issues related to notebooks performance issues related to performance labels Oct 2, 2024
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really impressive! The notebook editor is now way more responsive and a lot of the bugs (like output disappearing on moving cells, etc.) are now fixed. However, I found some issues/regressions with the changes:

  1. Some outputs cannot be rendered after reopening a notebook. I don't think we can fix this (VS Code also cannot, see this image). However, this leads to miscalculations in the output renderer logic and everything seems to be misaligned:
    image
  2. Clicking on a rendered Markdown cell (non-editing mode) does not focus the cell. One has to click on the sidebar of the cell instead. AFAIK this is a regression from the current master branch.
  3. Closing a notebook editor takes a surprising amount of time. In some cases more than a second. During this time, the application seems to freeze up. I can pretty consistently reproduce this using this notebook.

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Continuing the issues above:

  1. I've noticed some jittering in the layout while scrolling through a larger notebook. The editors seem to have a "second layout" phase where they increase their size by a few pixels. This leads to a lot of jittering during the scrolling due to resizing editors. IIRC this is also the case on master, so treat this as a low-prio issue for now.

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe I found another (pretty weird) issue:

  1. Open a notebook and clear one cell output (the document becomes dirty)
  2. Close the notebook (it doesn't matter whether you save the changes)
  3. Reopen the notebook - no outputs are visible
  4. Closing and reopening the notebook fixes the issue (?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notebook issues related to notebooks performance issues related to performance
Projects
Status: Waiting on reviewers
Development

Successfully merging this pull request may close these issues.

2 participants