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

Make Cells Display Lazily When Diffing Unchanged Cells #636

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

Conversation

gwincr11
Copy link
Contributor

@gwincr11 gwincr11 commented Oct 24, 2022

Motivation:
During diffing Unchanged cells are displayed, which is not necessary.
This is a performance issue when there are many unchanged cells.

Related Issues:

Open Questions:

  • I am struggling with getting the svg images to build properly.
  • How can this be tested in JupyterLab?

Changes:

  • Alter Notebooks Widget to only display changed cells by using new linked list of cells.
  • Add new linked list of cells and lazy version of linked list of cells.

Screen Shot 2022-10-24 at 12 33 59 PM

@gwincr11
Copy link
Contributor Author

gwincr11 commented Nov 8, 2022

@vidartf any hints on how to get the SVG to render properly? If not I can inline it 😅

@vidartf
Copy link
Collaborator

vidartf commented Nov 22, 2022

@vidartf any hints on how to get the SVG to render properly? If not I can inline it 😅

I'm unsure what the best approach is here. I think if we want import to work, it will be a breaking change, as any library that depends on nbdime will now have to figure out bundling rules for SVGs. I think also some care need to be taken about adding classes etc to the SVG elements, so that JupyterLab can theme it properly. I think it might be good to read up on https://jupyterlab.readthedocs.io/en/stable/extension/ui_components.html#how-to-create-your-own-custom-labicon

--jp-expand-outer-wrapper: rgb(221, 244, 255);
--jp-expand-color: rgb(87, 96, 106);
--jp-expand-a-color: rgba(84, 174, 255, 0.4)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: Ideally concepts of the DOM should not bleed into the CSS variable names. Ideally it would say something about the intended difference/relationship between the different variables, so that it can more easily by themed by others (and have those themes survive a change to the DOM).

Also, this color has an alpha element in order to work on multiple themed backgrounds. The other colors in these variables should ideally also work in the same constraints. Mainly since default JupyterLab themes do not include nbdime theming, but we still want nbdime to work with all of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I changed these to not reflect the dom elements that use them.

@gwincr11
Copy link
Contributor Author

I seem a bit stuck on this since typescript stoped compiling. I opened a pr to update typescript which will allow me to complete this pr, thanks

Motivation:
  During diffing Unchanged cells are displayed, which is not necessary.
  This is a performance issue when there are many unchanged cells.

Related Issues:
  - jupyter#635

Open Questions:
  - I am struggling with getting the svg images to build properly.
  - How can this be tested in JupyterLab?

Changes:
  - Alter Notebooks Widget to only display changed cells by using new linked
    list of cells.
  - Add new linked list of cells and lazy version of linked list of cells.
Copy link
Collaborator

@vidartf vidartf left a comment

Choose a reason for hiding this comment

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

Just doing a quick code-diff only review (still need to pull and build for manual checks):

nbdime/webapp/templates/diff.html Show resolved Hide resolved
@@ -0,0 +1 @@
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" width="24" height="24"><path fill-rule="evenodd" d="M12 19a.75.75 0 01-.53-.22l-3.25-3.25a.75.75 0 111.06-1.06L12 17.19l2.72-2.72a.75.75 0 111.06 1.06l-3.25 3.25A.75.75 0 0112 19z"></path><path fill-rule="evenodd" d="M12 18a.75.75 0 01-.75-.75v-7.5a.75.75 0 011.5 0v7.5A.75.75 0 0112 18zM10.75 6a.75.75 0 01.75-.75h1a.75.75 0 010 1.5h-1a.75.75 0 01-.75-.75zm-8 0a.75.75 0 01.75-.75h1a.75.75 0 010 1.5h-1A.75.75 0 012.75 6zm12 0a.75.75 0 01.75-.75h1a.75.75 0 010 1.5h-1a.75.75 0 01-.75-.75zm-8 0a.75.75 0 01.75-.75h1a.75.75 0 010 1.5h-1A.75.75 0 016.75 6zm12 0a.75.75 0 01.75-.75h1a.75.75 0 010 1.5h-1a.75.75 0 01-.75-.75z"></path></svg>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does these icons style in lab when the theme changes? (not able to do a build rn) If not, please check these docs: https://jupyterlab.readthedocs.io/en/stable/extension/ui_components.html#sync-icon-color-to-jupyterlab-theme

packages/nbdime/src/diff/widget/notebook.ts Show resolved Hide resolved
packages/nbdime/src/diff/widget/notebook.ts Show resolved Hide resolved
--jp-expand-color: rgb(87, 96, 106, 1);
--jp-expand-activate-color: rgba(84, 174, 255, 0.4);
--jp-expand-activate-focus-color: rgb(14, 94, 208, 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could these be re-written to be partially transparent, so that they have a fighting chance of working on both dark an light themes? Alternatively be based on JupyterLab CSS variables so that they might have inherent styling?

@vidartf
Copy link
Collaborator

vidartf commented Nov 10, 2023

I opened a rebased version of this PR in #733.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants