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

Upgrade nbdime for jupyterlab 4 #664

Conversation

HaudinFlorence
Copy link
Contributor

@HaudinFlorence HaudinFlorence commented Apr 26, 2023

This PR 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)

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
comparisons_diff_example8

Merge app rendering example
comparisons_merge_example8

Styling/UX differences

  • the collapse functionnality has been removed.
  • the new background is white when it was previously light grey
  • in the merge app, the padding spacers don't have colors. They are ll 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.)
  • in the diff app, the small arrows between editors have been removed. There are picker gutter markers in the diff too.
  • 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.
  • 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).
  • 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.
  • 'Metadata changed' panel at the end of the applications is missing.

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.

@HaudinFlorence HaudinFlorence force-pushed the upgrate_nbdime_for_jupyterlab_4 branch 3 times, most recently from 55b26b1 to 4a83fed Compare April 26, 2023 13:11
@HaudinFlorence HaudinFlorence changed the title Upgrate nbdime for jupyterlab 4 Upgrade nbdime for jupyterlab 4 Apr 26, 2023
@HaudinFlorence HaudinFlorence force-pushed the upgrate_nbdime_for_jupyterlab_4 branch from 99069d8 to 58238ca Compare April 27, 2023 15:13
.gitignore Outdated
@@ -12,6 +12,7 @@ MANIFEST
.cache
.pytest_cache
.idea
.yarn
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that this repo is currently using npm + package-lock.json, so please avoid adding yarn things.

Copy link
Contributor Author

@HaudinFlorence HaudinFlorence May 2, 2023

Choose a reason for hiding this comment

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

@vidartf Thanks for your feedback. I will change this for npm usage.

package.json Outdated
"lerna": "^4.0.0",
"rimraf": "^5.0.0"
}
"private": true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are a lot of formatting changes in this PR. It would be very much easier for me to look at it if pure formatting changes were removed! 😃

Copy link
Contributor Author

@HaudinFlorence HaudinFlorence May 2, 2023

Choose a reason for hiding this comment

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

@vidartf That's right. Would you be ok to add a formatter like prettier on the project? This could be done in a separate PR. If so I can do it. The changes related to the migration to jupyterlab 4 would then appear more clearly. Please let me know what you think about it. Thanks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The bottle neck for my work on nbdime is the time I have to spend on it. If this is just a config for prettier + an autoformat commit + the ignore config so that GH doesn't consider that commit for git blame, it would be ok. I don't want to have to spend time on managing commit hooks or CI actions related to formatting though. So it might just be simpler to undo the pure-formatting changes in this PR (they will also pollute the git blame as they are).

Thanks for your understanding 😃

Copy link
Contributor Author

@HaudinFlorence HaudinFlorence May 4, 2023

Choose a reason for hiding this comment

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

@vidartf Thanks for your reply. So we can try to do what you are suggesting : " a config for prettier + an autoformat commit + the ignore config so that GH doesn't consider that commit for git blame." As a first step, I opened this draft PR #668 with a config for prettier (that can be tuned), and a commit with the autoformat changes. I guess that we can add in a second time a .git-blame-ignore-revs so that Github does not consider this commit for git blame. Let me please know if the prettier config does not fit your expectations.

@HaudinFlorence HaudinFlorence force-pushed the upgrate_nbdime_for_jupyterlab_4 branch 5 times, most recently from 771968a to 616bb52 Compare May 2, 2023 15:22
@HaudinFlorence HaudinFlorence force-pushed the upgrate_nbdime_for_jupyterlab_4 branch from 44e6205 to a1e3402 Compare June 8, 2023 18:13
@HaudinFlorence HaudinFlorence force-pushed the upgrate_nbdime_for_jupyterlab_4 branch 2 times, most recently from f258f32 to 84e276e Compare June 15, 2023 08:50
@HaudinFlorence HaudinFlorence force-pushed the upgrate_nbdime_for_jupyterlab_4 branch 2 times, most recently from 9a178d2 to d4711df Compare June 30, 2023 15:17
@HaudinFlorence HaudinFlorence force-pushed the upgrate_nbdime_for_jupyterlab_4 branch from d4711df to 66c64a5 Compare June 30, 2023 15:22
@HaudinFlorence HaudinFlorence force-pushed the upgrate_nbdime_for_jupyterlab_4 branch from 6cbdb28 to 2530d06 Compare July 11, 2023 17:06
…), add a listener, try to reintroduce syntax highlighting (not yet working) and introduce minor css changes.
@HaudinFlorence HaudinFlorence force-pushed the upgrate_nbdime_for_jupyterlab_4 branch from 2530d06 to 8b0b49d Compare July 12, 2023 11:05
@HaudinFlorence HaudinFlorence force-pushed the upgrate_nbdime_for_jupyterlab_4 branch from a87e263 to 22f9f70 Compare September 8, 2023 08:22
@vidartf
Copy link
Collaborator

vidartf commented Sep 8, 2023

As I mentioned previously, I've been traveling recently, so I've missed a lot of the discussion here. I will try to read through the discussion, but for now: I merged the lint PR, so now @HaudinFlorence is no longer a first-time contributor, so workflows should no longer need manual approvals.

@vidartf
Copy link
Collaborator

vidartf commented Sep 8, 2023

Thanks @HaudinFlorence.

What would make it easiest for you to move forward:

* wait for the UI tests and [A list of small improvements HaudinFlorence/nbdime#12](https://github.com/HaudinFlorence/nbdime/pull/12) to be merged into the branch and only then merge this PR

* merge this PR as-is and retarget the remaining PRs against master branch in this repo?

Review-wise this is getting difficult to handle due to conversation length and number of files modified, but we can handle two more merges if that's easier.

If you want, I can create a new branch on this repo for now (~"lab4"). Will that help, or does the actions config need to be in the default branch?

@krassowski
Copy link
Member

It needs to be on the default branch I believe. I had created https://github.com/jupyter/nbdime/tree/3.2.x yesterday so I think we could merge this one into master (merge conflicts due to prettier apart).

@HaudinFlorence
Copy link
Contributor Author

HaudinFlorence commented Sep 8, 2023

@vidartf Thanks a lot for merging the lint PR. A new PR rebasing with master has been opened here #673 with the help of @fcollonval. Hoping that the ui tests will be green🤞🏻🟢

@SylvainCorlay
Copy link
Member

Closing as superseded by #673.

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.

8 participants