Support JupyterLab 4.0, port to Lumino 2 #673

HaudinFlorence · 2023-09-08T16:34:37Z

This PR is a rebase of #664. It includes fixes following the rebase with the PR adding the linter.
An updated description of the PR is reproduced just below

PR #664 is a first pass on trying a migration to jupyterlab4. It includes :

moving from Lumino 1 to 2
moving from JupyterLab 2 || 3 to 4 working with codemirror 6 (CM6)
adding a linter
adding some first ui-tests

Main changes
This PR includes among others the migration to CM6 of these features of the web applications:

lines and characters hilighting using a dedicated a stateField, stateEffects and specific decorations of CM6
spacers when addition/deletion of texts are made using a stateField, stateEffects, a builder and decoration widgets of CM6
picker and conflict gutter markers using dedicated stateFields and stateEffects
update of the different methods that enable to update the views of the editors and the models of the diffView.

Diff app rendering example

Merge app rendering example

Styling/UX differences
features to be restored

the collapse functionality has been removed. It may be re-introduced in a future PR. Not sure how difficult it will be with CM6 (opinions on this point is welcomed).
the new background is white when it was previously light grey: it disappears in the migration process but it should be rather easy to re-introduce.
in the merge app, the padding spacers don't have colors. They are all grey (using decoration widgets makes the previous css logics not work anymore, this should be changed to make the coloring possible like in the diff web application.)
'Metadata changed' panel at the end of the applications is missing. This should be restored.

chosen differences or differences related to CM6

all padding spacers are directly displayed on the view, there is no need to click on a picker gutter markers to make a spacer fully expend with its real sizing.
there is no picker gutter marker for spacers. In the merge editor, only the text chunks are shown, there is no spacer.
in the diff app, the small arrows between editors have been removed. There are picker gutter markers in the diff too.
the syntax highlighting seems to have priority on the character change highlighting (as shown for the sinus function in the central editor in the merge example above). (later erratum : the approach is mixed with both syntax highlighting and diff coloring, one of the 2 approaches should be chosen.)
there is only 1 gutter when there were 2 previously (1 for pickers and 1 for the conflict gutter markers)
there is a small white space between editors defined in the css of the grid panel now used to encapsulate the editors.

Additionnal comments on changes

adding @types/jest was helping with running the tests
disabling auto-detection of language server appears to fix the fact that the last Python test was failing. Test was working locally but was simply timing out on CI because search for language servers was taking long time on slow CI file system.
update-integration-tests.yml from the extension template has been added for updating snapshots.It enables to then just comment "please update playwright snapshots" text to trigger that workflow.
note that references to mathjax have been removed since MathJax4 implementation in JupyterLab 4 now ships all static assets by itself, so there is no need for it. Indeed The import in the server, from jupyter_server_mathjax.app import STATIC_ASSETS_PATH and related code have been removed too.

Future work and follow up tasks

In some cases, the result of the native function findAlignedLines is not right leading to a misalignment of lines to be put at the same height and to incorrect treatment of adding paddings. An issue has been opened with an example findAlignedLines sometimes returns unexpected results #669
More ui tests will be added when findAlignedLines will be fixed for the non working case
In reference to comment Upgrade nbdime for jupyterlab 4 #664 (comment), changes will have to be made after the PR is merged regarding use of find, each, toArray related to Lumino to move to JS equivalent.

krassowski · 2023-09-08T17:49:46Z

Hooray, green! Two suggestions:

Could you update the PR description (copy-pasting the description of Upgrade nbdime for jupyterlab 4 #664 would be good enough, but maybe there is something to add)
Since the commit count went from over 80 to less than 20, there are probably places where the intend of a change is no longer documented by the git history; if would be good to add comments in these places. For example, the reason for disabling auto-detection of language servers.
- while you are at cleaning the git history I would note that, a commit named "Update alignViews in mergeview.ts to correct padding issues (only the…" appears twice.

HaudinFlorence · 2023-09-08T20:51:38Z

@krassowski Thanks for your suggestion. I have addressed the first point concerning the PR description. The duplicated commit has been squashed.

HaudinFlorence · 2023-09-09T10:02:04Z

Since the commit count went from over 80 to less than 20, there are probably places where the intend of a change is no longer documented by the git history; if would be good to add comments in these places. For example, the reason for disabling auto-detection of language servers.

@krassowski Sorry if it is obvious, but where do you think I should document theses changes?

…), add a listener, try to reintroduce syntax highlighting (not yet working) and introduce minor css changes.

Update onGutterClick method, syncModel Update of the mergeView Perform some renaming Add methods to remove the line-chunk mapping and the gutter markers Refactoring of the code : remove most of create effect methods except for the gutter effects.

Choose the extensions to be added from Jupyterlab default ones Migrate the synchronized scrolling Add transparent borders to the chunk decorations Update mergeview.ts and relative styling to make nbdiff work

… diff is implemented) Update css files

Update mergeview.ts and editor.ts with minor changes Update example9 files. Update mergeview.ts to take review comments into account. Update css files with minor changes. Update to take review into account. Keep on taking review into account regarding the iterator issue, dependencies and CI troubles. Add jest types Try fix CI js failing tests and try to fix PatchObjectHelper iterator. Remove any reference to jupyter_server_mathjax and related references. Fix iterator implementation on `PatchObjectHelper` Fix tests Fix Python test failing due to retained decorator Remove one more remnant of old mathjax Disable auto-detection of language servers to reduce startup time Add ui-tests to github workflows. Add missing directory with example notebooks data/default/test2. Try to make ui-tests work. Update ./github/workflows/tests.yml Update .github/workflows/tests.yml Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com> Remove last commit change adding option --update-snapshots for npx playwright test and add update-integration-tests.yml for to .github/workflows. Update .github/workflows/tests.yml Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com> Update .github/workflows/update-integration-tests.yml Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com> Take comments into account regarding test.yml file and tests. Apply suggestions from code review Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com> Remove `set -eux` because it is Unix-only Use npm client Actually pass python version to setup script Add missing `run` Add comment/reaction steps Add polyfill for playwright npx run Only update snapshots on latest Python, run one at a time to avoid git conflicts Update Playwright Snapshots Apply suggestions from code review Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com> Restore green colors backgrounds for the right editor for a diff. Fix the added cells CSS style. Fix alignment shift for diff in alignViews. Fix `lineChunks` and editor configuration, including readOnly Remove hard-coded `lineNumbers` option Update mergeview.ts and diff.css to try to fix the added or deleted cells backgrounds. Take comment into account to fix the background of added/deleted cells. Restore `getMergedValue()` Restore border indicator, remove unused .CodeMirror selector Add ui-tests for diff and change ui-tests names and descriptions. Fix dep specification Restore CSS scoping Reduce diff due to format changes Restore `getMergedValue` Improve integration tests Improve integration tests Avoid duplicating example/test contents Instantiate registries only once Only create editor factory (and associated registries) once Don't remove favicon when cleaning Remove old typings Fix style not deduplicated Use only prebuilt extension Fix jobs config Fix jest tests Fix integration tests Fix wheel name for windows Pass wheel name to ui-test job Update symlink Update Playwright Snapshots Update Playwright Snapshots Update ui-tests/tests/nbdime-merge-test1.spec.ts

krassowski · 2023-09-09T11:52:12Z

I would just add inline comments for cases where the internet may not be obvious.

HaudinFlorence · 2023-09-10T22:00:02Z

I would just add inline comments for cases where the internet may not be obvious.

I have added a section in the description of the PR called "Additionnal comments on changes" and a commit will be added with this comments added in the code too. Please feel free to mention what important points should be added from your point of view @krassowski @fcollonval @JohanMabille. Thanks.

fcollonval

Thanks a lot to all of you and especially @HaudinFlorence

ellisonbg

Thanks for working on this, it will be super helpful to have nbdime on JupyterLab 4!

krassowski · 2023-09-13T19:57:00Z

During the lab call @vidartf asked about dark theme. I can confirm that for JupyterLab diff mode works in dark mode.

Lab 3.0	This PR

It would be good to restore the background of cells in dark mode, which in my opinion could be in a follow-up PR. The comments on changed lines are hard to read bu this is not a regression.

vidartf

Initial diff scan of the files that do not include editor/mergeview. Doing this first, as it should be quicker to discuss questions for these.

nbdime/tests/conftest.py

nbdime/tests/test_server_extension.py

nbdime/webapp/nbdimeserver.py

package.json

packages/labextension/package.json

packages/webapp/src/style/index.js

setup.py

tsconfig.json

tsconfig_base.json

vidartf · 2023-09-15T11:57:34Z

examples/example1

@@ -0,0 +1 @@
+../ui-tests/data/merge_test1


What are the files in this examples folder meant to be used for? How does it differ from the files in the ui-tests folder, and the nbdime/tests/files folder?

Maybe we could reduce the number of examples in the dedicated folders.They were used during the migration process to check various diff and merge configurations.
The diff in ui-tests/diff_test1 and ui-tests/diff_test2 are variants of example1 (center.ipynb with addition/deletions of additionnal cells) and ui-tests/diff_test3 is a variant of example8 (with left.ipynb and center.ipynb notebooks).
The merge in ui-tests/merge_test1 and ui-tests/merge_test2 are variant of example1 and ui-tests/merge_test3 is a variant of example3.

nbdime/tests/files folder has nothing in common with ui-tests and examples.

Co-authored-by: Vidar Tonaas Fauske <vidartf@gmail.com>

vidartf · 2023-09-15T13:54:09Z

Re the OP's "Styling/UX differences" sections. Is that up to date? Would you mind clarifying which of these points are changed because of technical challenges, and which (if any) are considered improvements? E.g. why was the ability to have collapsed sections removed, and what would the challenges be to add it back in?

HaudinFlorence · 2023-09-15T15:58:09Z

Re the OP's "Styling/UX differences" sections. Is that up to date? Would you mind clarifying which of these points are changed because of technical challenges, and which (if any) are considered improvements? E.g. why was the ability to have collapsed sections removed, and what would the challenges be to add it back in?

@vidartf many thanks for your review. The different comments should be addressed the commit labelled as 'Take review comments into account' 63c9c24. Concerning the styling/UX differences section, it is up to date and I have clarified these differences by classified them between two sections : the first one is entitled "features to be restored" and the second one is named "chosen differences or differences related to CM6". Feel free to comment if you think that some missing features have to be restored (e.g. the smalls arrows in the diff application ?) or if some may be given up (like coloring the spacers in the merge application ?).

nbdime/webapp/nbdimeserver.py

packages/labextension/style/index.css

vidartf · 2023-09-15T16:45:04Z

packages/nbdime/src/diff/widget/metadata.ts

@@ -18,8 +22,13 @@ const ROOT_METADATA_CLASS = 'jp-Metadata-diff';
 * MetadataWidget for changes to Notebook-level metadata
 */
 export class MetadataDiffWidget extends Panel {
-  constructor(model: IStringDiffModel) {
+  // TODO improve typing hierarchy to avoid `Omit`


Is this TODO for yourself, or for someone in the future? :)

I don't think this comment is mine so I don't know.

I added it - I will remove it when working into the integration with @jupyterlab/git

fcollonval · 2023-09-18T07:49:57Z

@vidartf are you concerns addressed? Could we merge this PR?

Could you also accept the NPM invitation to join the jupyter organization? Once accepted I will have to increase your rights for you to add the nbdime package as it is not possible to grant new member advanced rights by default.

vidartf · 2023-09-18T09:12:38Z

@fcollonval My previous first-pass concerns are indeed addressed, and I've convinced myself that the styling concerns I have can be iterated on in follow-up PRs (I have fixes for most of them locally). My current major concern is that the mergetool seems to be extremely unreliable (clicking the pickers sometimes work, and sometimes not at all), and I am trying to figure out exactly what is causing this. I'm going to spend some time today to understand it, and how much work a fix would entail, then I'll get back to you.

vidartf · 2023-09-18T09:24:08Z

Never mind the mergetool. The examples I found that broke it seems to be broken in previous releases of nbdime as well. I will create an issue for that separately.

vidartf · 2023-09-18T09:32:46Z

(actually, the mergetool is more generally broken in this PR than previous releases, but I think we can merge and iterate on that...)

SylvainCorlay · 2023-09-18T09:59:47Z

Congrats to @HaudinFlorence, this is a significant achievement!

Many thanks to everyone involved (@vidartf @krassowski @fcollonval @JohanMabille).

vidartf · 2023-09-18T10:01:03Z

Yes, thanks again @HaudinFlorence .

vidartf · 2023-09-18T11:00:19Z

@HaudinFlorence I'm not sure if I fully understood what this meant. Can you help clarify?

Metadata changed' panel at the end of the applications is missing. This should be restored.

HaudinFlorence · 2023-09-18T13:34:16Z

@HaudinFlorence I'm not sure if I fully understood what this meant. Can you help clarify?

Metadata changed' panel at the end of the applications is missing. This should be restored.

@vidartf It seemed to me that in the case of the merge app (mostly when there are more than one cells), the blue indication after the cell mentioning "Metadada changed" was missing. I am adding a capture of how it looks in CM5 version (not able to build CM6 version right now.)

HaudinFlorence force-pushed the nbdime_upgraded branch from 5f8c05d to 0f79987 Compare September 8, 2023 16:41

HaudinFlorence mentioned this pull request Sep 8, 2023

Upgrade nbdime for jupyterlab 4 #664

Closed

HaudinFlorence and others added 16 commits September 9, 2023 12:04

Update dependencies for jupyterlab 4.

43b0608

EditorWrapper migrated to CM6 and started mergeview

79414bb

Update mergeview.ts and other files.

6f65fd4

Update dependencies.

1f80d1d

Fixed build

9cce097

Migrate highlighting to CM6.

f6d431b

Migrate padding to CM6.

78cd4fd

Keep on migrating padding (one effect by editor using decoration sets…

d461c0f

…), add a listener, try to reintroduce syntax highlighting (not yet working) and introduce minor css changes.

Migrate gutter markers.

6d97463

Keep on migrating gutter markers

8156966

Update onGutterClick method, syncModel Update of the mergeView Perform some renaming Add methods to remove the line-chunk mapping and the gutter markers Refactoring of the code : remove most of create effect methods except for the gutter effects.

Restore the missing syntax highlighting

7bd87c9

Choose the extensions to be added from Jupyterlab default ones Migrate the synchronized scrolling Add transparent borders to the chunk decorations Update mergeview.ts and relative styling to make nbdiff work

Update alignViews in mergeview.ts to correct padding issues (only the…

660f8ef

… diff is implemented) Update css files

Fix spacers algorithm

ec92d93

Fix last spacer

414eb76

Fix following rebase.

711fa95

HaudinFlorence force-pushed the nbdime_upgraded branch from 0f79987 to 711fa95 Compare September 9, 2023 10:06

SylvainCorlay approved these changes Sep 11, 2023

View reviewed changes

JohanMabille approved these changes Sep 11, 2023

View reviewed changes

krassowski approved these changes Sep 11, 2023

View reviewed changes

Add comments about changes or configurations.

ecc4478

HaudinFlorence force-pushed the nbdime_upgraded branch from cdcd7a4 to ecc4478 Compare September 11, 2023 09:10

fcollonval approved these changes Sep 11, 2023

View reviewed changes

ellisonbg approved these changes Sep 12, 2023

View reviewed changes

vkaidalov mentioned this pull request Sep 13, 2023

Upgrade to jupyterlab 4 jupyterlab/jupyterlab-git#1236

Merged

vidartf reviewed Sep 15, 2023

View reviewed changes

Update packages/labextension/src/actions.ts

c721222

Co-authored-by: Vidar Tonaas Fauske <vidartf@gmail.com>

Take review comments into account.

63c9c24

vidartf reviewed Sep 15, 2023

View reviewed changes

nbdime/webapp/nbdimeserver.py Outdated Show resolved Hide resolved

vidartf reviewed Sep 15, 2023

View reviewed changes

packages/labextension/style/index.css Outdated Show resolved Hide resolved

vidartf reviewed Sep 15, 2023

View reviewed changes

Fix trailing comma and indentation issues.

f3e4821

Remove broken examples

872a71c

vidartf merged commit da240a2 into jupyter:master Sep 18, 2023
8 checks passed

This was referenced Sep 18, 2023

Migration to jupyterlab 4.0 #659

Closed

CM6: Fix missing mergeview pickers #676

Closed

CM6: Investigate syntax highlighting in diff chunks #678

Closed

This was referenced Sep 18, 2023

CM6: Restore collapser #679

Closed

Converge on API to be able to reuse nbdime diff and merge widgets for plain text in @jupyterlab/git #681

Closed

krassowski mentioned this pull request Sep 21, 2023

Fix wrong picker marker (a typo during lab 4.0 migration) #688

Merged

fcollonval mentioned this pull request Sep 25, 2023

Vertical spacer outside of viewport are not included #693

Open

mathis-marcotte mentioned this pull request Nov 2, 2023

JupyterLab: Investigate disabled extensions StatCan/aaw-kubeflow-containers#549

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support JupyterLab 4.0, port to Lumino 2 #673

Support JupyterLab 4.0, port to Lumino 2 #673

HaudinFlorence commented Sep 8, 2023 •

edited

Loading

krassowski commented Sep 8, 2023

HaudinFlorence commented Sep 8, 2023 •

edited

Loading

HaudinFlorence commented Sep 9, 2023 •

edited

Loading

krassowski commented Sep 9, 2023

HaudinFlorence commented Sep 10, 2023 •

edited

Loading

fcollonval left a comment

ellisonbg left a comment

krassowski commented Sep 13, 2023

vidartf left a comment

vidartf Sep 15, 2023

HaudinFlorence Sep 15, 2023

vidartf commented Sep 15, 2023

HaudinFlorence commented Sep 15, 2023 •

edited

Loading

vidartf Sep 15, 2023

HaudinFlorence Sep 15, 2023 •

edited

Loading

fcollonval Sep 18, 2023

fcollonval commented Sep 18, 2023

vidartf commented Sep 18, 2023

vidartf commented Sep 18, 2023

vidartf commented Sep 18, 2023

SylvainCorlay commented Sep 18, 2023

vidartf commented Sep 18, 2023

vidartf commented Sep 18, 2023

HaudinFlorence commented Sep 18, 2023 •

edited

Loading

		@@ -0,0 +1 @@
		../ui-tests/data/merge_test1

Support JupyterLab 4.0, port to Lumino 2 #673

Support JupyterLab 4.0, port to Lumino 2 #673

Conversation

HaudinFlorence commented Sep 8, 2023 • edited Loading

krassowski commented Sep 8, 2023

HaudinFlorence commented Sep 8, 2023 • edited Loading

HaudinFlorence commented Sep 9, 2023 • edited Loading

krassowski commented Sep 9, 2023

HaudinFlorence commented Sep 10, 2023 • edited Loading

fcollonval left a comment

Choose a reason for hiding this comment

ellisonbg left a comment

Choose a reason for hiding this comment

krassowski commented Sep 13, 2023

vidartf left a comment

Choose a reason for hiding this comment

vidartf Sep 15, 2023

Choose a reason for hiding this comment

HaudinFlorence Sep 15, 2023

Choose a reason for hiding this comment

vidartf commented Sep 15, 2023

HaudinFlorence commented Sep 15, 2023 • edited Loading

vidartf Sep 15, 2023

Choose a reason for hiding this comment

HaudinFlorence Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

fcollonval Sep 18, 2023

Choose a reason for hiding this comment

fcollonval commented Sep 18, 2023

vidartf commented Sep 18, 2023

vidartf commented Sep 18, 2023

vidartf commented Sep 18, 2023

SylvainCorlay commented Sep 18, 2023

vidartf commented Sep 18, 2023

vidartf commented Sep 18, 2023

HaudinFlorence commented Sep 18, 2023 • edited Loading

HaudinFlorence commented Sep 8, 2023 •

edited

Loading

HaudinFlorence commented Sep 8, 2023 •

edited

Loading

HaudinFlorence commented Sep 9, 2023 •

edited

Loading

HaudinFlorence commented Sep 10, 2023 •

edited

Loading

HaudinFlorence commented Sep 15, 2023 •

edited

Loading

HaudinFlorence Sep 15, 2023 •

edited

Loading

HaudinFlorence commented Sep 18, 2023 •

edited

Loading