jupyterlab / jupyterlab

JupyterLab computational environment.
https://jupyterlab.readthedocs.io/
Other
13.82k stars 3.12k forks source link

Fix dirty dots not preserved when moving multiple cells #16225

Closed Alanhou1222 closed 2 weeks ago

Alanhou1222 commented 3 weeks ago

References

Fixes https://github.com/jupyterlab/jupyterlab/issues/16060 Closes https://github.com/jupyterlab/jupyterlab/pull/16161 (includes)

Code changes

In packages/cells/src/model.ts, make the setDirty() function into a set function set isDirty(). In packages/notebook/src/widget.ts, move logics that set new cells dirty into the for loop of the moveCell function and make the code more clear.

User-facing changes

dirty dots are kept even when moving multiple cells image image

Backwards-incompatible changes

None

jupyterlab-probot[bot] commented 3 weeks ago

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

Alanhou1222 commented 3 weeks ago

@krassowski Not sure if it's expected, but moved cells won't remember their content before being moved. Therefore, even if users revert the changes on a moved cell, the cell is still considered dirty.

krassowski commented 3 weeks ago

but moved cells won't remember their content before being moved

Indeed, undo does not work after moving a cell - good catch! This is not expected, we should open a separate issue to track it and then fix it.

This PR looks good to me on the implementation side (the failing Python tests are unrelated); ideally we would also include unit tests in packages/notebook/test/widget.spec.ts for the dirty dot being preserved. Would you be interested in adding such a unit test (whether in this PR or in another)?

Alanhou1222 commented 3 weeks ago

but moved cells won't remember their content before being moved

Indeed, undo does not work after moving a cell - good catch! This is not expected, we should open a separate issue to track it and then fix it.

This PR looks good to me on the implementation side (the failing Python tests are unrelated); ideally we would also include unit tests in packages/notebook/test/widget.spec.ts for the dirty dot being preserved. Would you be interested in adding such a unit test (whether in this PR or in another)?

Sure, I'll write some unit tests.

Alanhou1222 commented 3 weeks ago

@krassowski I added two tests, one verifies dirty state is preserved after moving one code cell and another verifies dirty states are preserved after moving multiple cells. Let me know if you think there are other situations that should be tested.