Update skipped/disabled cell dependency before saving notebook

[disberd]

The load_notebook function loaded and re-saved the notebook if not called with a flag to disable writing. Before this PR, since the indirect dependency of disabled and skipped cells was not computed (probably because in most cases when a notebook is loaded is also then somehow executed so the dependency is computed after), the save to file would remove the block comments to all cells that were not directly skipped or disabled, breaking the intended functionality and creating errors when including Pluto notebooks as scripts.

Fixes https://github.com/fonsp/Pluto.jl/issues/2182

I did not put the update functions inside load_notebook_nobackup as they might create some overhead that is not needed in most cases (I computed only 40-50 μs on a fairly large notebook in a simple benchmark)

[github-actions[bot]]

Try this Pull Request!

Open Julia and type:

julia> import Pkg
julia> Pkg.activate(temp=true)
julia> Pkg.add(url="https://github.com/disberd/Pluto.jl", rev="load_notebook_fix")
julia> using Pluto

[Pangoraw]

Without running the notebook the topology might loose dependency information regarding macros and therefore may not be able to re-disable the same cells. Another solution to make sure that the cells loaded as dependent on skipped/disabled are still saved as such is using the information given by the notebook file:

https://github.com/fonsp/Pluto.jl/blob/5bc4a813a5fbc21a915632ef8bca29bbe1785fc9/src/notebook/saving%20and%20loading.jl#L203-L223

[disberd]

@Pangoraw, I have seen your referenced commit and I have just one clarification. Do you mean just having a single field to record the depends_on_something on the julia back-end and then distinguishing between what is the cause of the indirect dependence on the front-end? Because the behavior on the front-end is different depending on whether the cell depends on a disabled or skipped one

[Pangoraw]

Yes the UI is one problem with my approach, so perhaps there should be a marker for those cells that is disabled on the first run (maybe not ideal) ?

My point is preventing the notebooks from stopping to work as a script when using Symbolics.jl for example with activate_notebook_environment() for example.

[disberd]

@Pangoraw, what do you think about a shift in the way we track all these status flags of a cell? Now we have basically 4 different flags for disabled, depends_on_disabled, skipped, depends_on_skipped. I suppose at some point we might want/need to add an additional status. What if we migrate from separate flags to maybe a single field that is a Set of strings or symbols representing a status. That way it's much easier to add potentially additional status and you have a single field in the Cell that contains all currently existing statuses, with checking for a specific field becoming checking for presence of the status name in the Set.

For the specific problem of this PR, we could have an additional generic state which is like :commented_on_file, which would be added whenever something is depending on skipped or disabled, but we could also just create it when loading the notebook as per your suggestion above.

I could try a separate PR for that unless you or others see something that would already be a deal-breaker with this approach. Checking for inclusion in a Set is slower than checking for the value of a specific field, but it's still in the range of 10-20ns in my laptop, which I believe is still significantly faster than needed for the purpose at hand.

[Pangoraw]

You're right that it would require a new state for a cell (that can simple be removed when the cell is first run).

What if we migrate from separate flags to maybe a single field that is a Set of strings or symbols representing a status.

What would be the advantage over adding cell fields ? Ultimately, I'd like these properties to be more tied to the topology than the cell itself.

[disberd]

I just think that having a single status field is neater and probably easier to maintain (especially if we keep adding flags/status when adding functionality to Pluto) than having a separate bool field for each possible status the cell can have. A potential nice thing would be to like automatically create HTML classes from the status Set/Dict based on the keys that are present rather than independently checking for each single flag explicitly.

[Pangoraw]

Let's go with this approach for now, this can be made more robust in the future if needed (recovering the same notebook file). Sorry for the delay and thanks for the work @disberd :tada:

[fonsp]

~Does this still work with https://github.com/fonsp/Pluto.jl/pull/2303 ? I suspect that the tests in that PR won't pass if you change update_run to update_save_run in the tests, because this PR runs the old algorithm again.~ Never mind it doesn't affect thesave_notebook function. 👍

This PR uses the algorithm that we used before https://github.com/fonsp/Pluto.jl/pull/2303. Is that a problem?

[fonsp]

@disberd @Pangoraw