Keyboard shortcuts related to collapsible headings

[schmidi314]

References

#10785
Fixes #11401

Adding commands and callbacks for features similar to Collapsible Headings in classic notebook:

• notebook:move-cursor-heading-above-or-collapse
• notebook:move-cursor-heading-below-or-expand
• notebook:insert-heading-above
• notebook:insert-heading-below

Code changes

IMHO packages/notebook/src/actions.tsx could use some more structure, at least if it comes to the private functions. For the moment, I have created a new namespace Private.Headings that contains all new functions related to headings. I think this one:

jupyterlab/packages/notebook/src/actions.tsx

Line 1622 in a6c19ed

function findNearestParentHeader(

should be moved into this namespace as well... or, since findNearestParentHeader seems to be buggy (fails if cell is a markdown cell), it should be replaced by the new

jupyterlab/packages/notebook/src/actions.tsx

Line 2425 in a6c19ed

export function findParentHeading(

...still investigating

User-facing changes

• Default shortcuts for ArrowLeft (jump to heading above or collapse) and ArrowRight (jump to heading below or expand)

• Default shortcuts for Shift+A (new heading of same level above) and Shift+B (new heading of same level below)

• Better cursor placing when starting to edit a heading cell.

[jupyterlab-probot]

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link:

[schmidi314]

@meeseeksdev tag enhancement

[fcollonval]

Thanks for starting this @schmidi314

A comment about GitHub, when referring to an issue, could you set the special word fix or fixes or closes in front of the #<ref>? This will automatically link the issue with the PR (and close it when the PR is merged).

This PR addresses also partly #11401 (actually to fix that issue the only thing missing are shortcuts Ctrl Shift Left/Right to (un)collapse all headings).

[fcollonval]

This looks awesome @schmidi314; I tested it on binder and this works as expected.

I pinged Martha that introduces the heading collapser feat. about avoiding code duplication and having common helper functions (I'm not familiar with that part of the code).

Would you mind adding some test for it?

about the keyboard test with Playwright, you can emulate key event with page.keyboard

[schmidi314]

Would you mind adding some test for it?

Sure. I was hesitant to do that because the UI tests already take ages. But I'll add some soon.

This PR addresses also partly #11401 (actually to fix that issue the only thing missing are shortcuts Ctrl Shift Left/Right to (un)collapse all headings).

Then I probably should add them as well.

[fcollonval]

Would you mind adding some test for it?

Sure. I was hesitant to do that because the UI tests already take ages. But I'll add some soon.

The value of those tests is huge. But I know that designing them takes time. One tip on that front is to run only one test file with:

jlpm run test tests/jupyterlab/workspace.test.ts

Or even a single test with the -g option:

jlpm run test -g "should reset the workspace"

This PR addresses also partly #11401 (actually to fix that issue the only thing missing are shortcuts Ctrl Shift Left/Right to (un)collapse all headings).

Then I probably should add them as well.

If you have the time, it would be awesome.

[schmidi314]

The value of those tests is huge. But I know that designing them takes time.

Yes, sure. :) But what I meant is that the UI test on github runs for about an hour already. At some point, I could imagine, automated tests become very annoying if you have to wait for half a day until completion. Maybe one should think about parallelization...?

And for some reason the snapshots on my computer don't agree with the tests on github. Maybe this is related to a non-100% scaling of my monitor. Any experience in this regard?

[schmidi314]

The Ctrl+Shift+Left/Right handlers and commands were already there, so that's an easy one.

I noticed that the notebook action collapseAll and the commands Collapsible_Headings:XXX don't quite match the scheme. I have marked the commits as optional to show my proposal to change them. Not sure if this actually improves things...

[fcollonval]

Yes, sure. :) But what I meant is that the UI test on github runs for about an hour already. At some point, I could imagine, automated tests become very annoying if you have to wait for half a day until completion.

GitHub won't let us reach that state as a CI job must complete in less than 6 hours 😉 (what is ok in comparison with the usual review time).

Maybe one should think about parallelization...?

When introducing the new galata based on Playwright fixture, I had more flaky test with multiple runner (and even sometimes it took longer...). But sure we should definitely try again especially as the number of tests grows.
An alternative can be to group tests and run some only when a PR is ready for review or something like that.

And for some reason the snapshots on my computer don't agree with the tests on github. Maybe this is related to a non-100% scaling of my monitor. Any experience in this regard?

Which OS are you using? Could you please provide a list of the failing tests?
Some tests are expected to fail (like the one related to the terminals) as they are influenced by the OS configuration. But we should reduce that list if possible.

[fcollonval]

I noticed that the notebook action collapseAll and the commands Collapsible_Headings:XXX don't quite match the scheme. I have marked the commits as optional to show my proposal to change them. Not sure if this actually improves things...

Thanks for adding these changes.

The PR is targeting v4. So it is fine for me especially because indeed it removes an inconsistency; if you want to highlight that in the documentation, you can add a note in the migration guide.

[schmidi314]

Which OS are you using? Could you please provide a list of the failing tests?

It would probably be easier to provide a list of those that don't fail :)

But maybe this helps: I think it is related to fonts (forget my last comment about resolution scaling). Typical cases:

In principle I'm on Ubuntu 20.04. But it is a customized OS shipped with a tuxedo laptop.

My impression is that raw and code cells are OK. For instance, the notebook-toolbar/cut-cell is fine (one code and one raw cell in the snapshot).

[fcollonval]

doing a test.use(mockSettings:...) overwrites all the DEFAULT_SETTINGS, it seems.

Yes they do, because we should not block the ability to test the settings defined in DEFAULT_SETTINGS. But you can always merge the default with new ones.

[fcollonval]

Thanks a lot for pushing this through @schmidi314

[github-actions]

Benchmark report

The execution time (in milliseconds) are grouped by test file, test type and browser.
For each case, the following values are computed: min <- [1st quartile - median - 3rd quartile] -> max.

The mean relative comparison is computed with 95% confidence.

Results table

Test file	large_code_notebook	large_md_notebook
open
chromium
actual	5232 <- [5640 - 5765 - 5944] -> 6375	3358 <- [3644 - 3833 - 4002] -> 4474
expected	4925 <- [5516 - 5691 - 5891] -> 6217	3366 <- [3651 - 3770 - 3882] -> 4193
Mean relative change	1.8% ± 1.2%	1.7% ± 1.5%
switch-from
chromium
actual	691 <- [884 - 947 - 988] -> 1312	546 <- [623 - 669 - 703] -> 928
expected	733 <- [868 - 912 - 981] -> 1209	549 <- [632 - 663 - 686] -> 869
Mean relative change	1.7% ± 2.8%	0.9% ± 2.3%
switch-to
chromium
actual	397 <- [480 - 512 - 536] -> 711	315 <- [375 - 398 - 422] -> 474
expected	357 <- [470 - 496 - 520] -> 607	301 <- [358 - 392 - 416] -> 458
Mean relative change	3.2% ± 2.4%	3.3% ± 2.6%
close
chromium
actual	716 <- [856 - 942 - 1308] -> 1425	533 <- [636 - 664 - 683] -> 803
expected	751 <- [853 - 965 - 1274] -> 1384	555 <- [630 - 652 - 674] -> 734
Mean relative change	1.1% ± 6.1%	1.1% ± 1.6%

Changes are computed with expected as reference.

[krassowski]

This is great, thank you. One question: how should insert new heading below behave when executed above the first heading/in empty notebook? The current behaviour seems a bit strange to me:

[schmidi314]

Ah, that was a refactoring bug. Thanks for noticing! ...so much for the test suite.

Nevertheless, since this is a corner case in my view, I wouldn't add tests for that.

[fcollonval]

Thanks @schmidi314

CI failure is not related.

[krassowski]

This looks amazing, thank you. Two points on the code:

• I spotted a debugging console.log - let's remove it
• I would personally avoid using idx abbreviation in any public API (e.g. I would rename returnIdx → returnIndex), but this is not a blocker; however, at a minimum please expand idx to index in documentation strings

[schmidi314]

Sorry, I've been away for a while. Anything left to do for me here?

[fcollonval]

Thanks @schmidi314 (especially for the ping), merging it now.