Sync dirty property between clients

[hbcarlos]

Solves a minor bug with the dirty property when saving the document.

References

I think we never opened an issue, and I can not find the issue/PR where this bug was detected, but @jasongrout detected a bug in the dirty property while working on something else.

Code changes

Sync the dirty property using the SharedDocument and improve the initialization.

User-facing changes

Now when someone saves the document, this change is notified to every client.

Backwards-incompatible changes

Not sure. Added a new property in SharedDocument and removed a private one from DocumentWidget.

[@fcollonval] This is ok as you added something to the public API (no changes, nor deletion)

[jupyterlab-probot]

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

[fcollonval]

Thanks @hbcarlos

[krassowski]

@hbcarlos could you add a gif demo? I think it did not work for me on binder yesterday.

[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	3775 <- [3897 - 3945 - 4628] -> 5307	2484 <- [2534 - 2566 - 2613] -> 3244
expected	3762 <- [3886 - 3954 - 4635] -> 5031	2467 <- [2530 - 2557 - 2611] -> 2848
Mean relative change	0.2% ± 2.8%	1.0% ± 1.1%
switch-from
chromium
actual	536 <- [565 - 593 - 670] -> 854	389 <- [425 - 478 - 518] -> 611
expected	522 <- [554 - 578 - 630] -> 829	389 <- [419 - 447 - 512] -> 591
Mean relative change	2.9% ± 3.8%	2.0% ± 3.1%
switch-to
chromium
actual	622 <- [640 - 651 - 667] -> 809	469 <- [532 - 548 - 558] -> 623
expected	619 <- [640 - 653 - 669] -> 797	476 <- [527 - 551 - 562] -> 613
Mean relative change	0.1% ± 1.4%	-0.5% ± 1.8%
close
chromium
actual	572 <- [623 - 673 - 928] -> 1086	442 <- [465 - 485 - 552] -> 600
expected	557 <- [616 - 667 - 925] -> 1048	442 <- [466 - 479 - 501] -> 604
Mean relative change	2.4% ± 6.0%	1.4% ± 2.5%

Changes are computed with expected as reference.

[hbcarlos]

Sure, I recorded a screencast from binder:

dirty-property.mp4

1 - When saving the document from a client, it changes for every client.
2 - When opening a file the dirty property stays false.
3 - When creating a new file the dirty property is true because the metadata changes (kernel-related stuff).

[krassowski]

Sorry about the delay here. So it does not seem to work for me on Binder for a notebook which was created before sharing the link:

Nor for a new notebook - it only updates indicator for one of the clients:

The behaviour does not appear consistent - sometimes it updates indicator on one side sometimes on the other, and once it even worked for both - but in any case it looks like something is wrong here.

So it seems that it was the git log still outputting the text and synchronization so the state of other notebooks could not be updated because of the live output.

[krassowski]

Please ignore the repro above it was tainted by the use of unconstrainted git log (see edit above - I should have used git log | head). The correct repro is below:

Sorry about the bad recording quality. Maybe the state is not synced properly when opening the notebook? It looks like the [1] prompt is missing in the newly opened notebook.

[hbcarlos]

Wow! Thanks for caching that one @krassowski!
I'll take a look, to see what is happening.

[hbcarlos]

The problem was that the dirty className was added multiple times to the title and removed once.
I added multiple checks to make sure we add it only once.

[krassowski]

Looks good on binder now, thanks!

[fcollonval]

@meeseeksdev please backport to 3.2.x

[dmonad]

@hbcarlos We should avoid parsing to the concrete Y* implementations because that is non-ideomatic.

In general, when you work with an abstraction, you shouldn't cast to the concrete implementation just to manipulate properties of the concrete implementation because that will eventually lead to issues.

In this case, we explicitly don't want to initialize shared properties whenever a user joins a session. Properties on the concrete Yjs implementation must only be initialized on the create and createStandalone methods.