saving / loading disable status of cells

[lungben]

implements #1205

• Saving and loading directly and indirectly disabled cells
• Make sure that indirectly disabled cells are updated before saving the notebook

[pankgeorg]

Is hiding the #s in the plan for the frontend?

[lungben]

I have not changed the way how deactivated cells look in the frontend.
The addition is that the cell deactivation information is now stored persistently in the notebook.jl file as commenting out the cell code with a special syntax.
Furthermore, the downstream dependencies are commented out, too (with a different marker) so that the notebook.jl file still runs stand-alone.

[ederag]

Upon reloading, the # are not visible in the frontend, good.

For now, after disable, quit and reopen, it seems to work fine, congratulations !
And re-enabling brings back exactly the previous file, perfect.

I really like how the manually disabled cells are darker than their dependencies.

A detail: in the file, the comments are dim, which is good (as in the frontend).
But some characters are not displayed here (kate editor, hack font):

This might be a kate limitation; "liberation mono" and "droid" have the same issue.
Another proposition, using the double line symbols that are used in other places by Pluto:

# ╔═╡ d5c3ed28-8854-4381-bcf3-9f764af94643
#=╠═ disabled
sol = solve(prob, Tsit5(), reltol=1e-8, abstol=1e-8)
  ╠═ disabled =#

I left the last tee symbol (and not a corner) just in case it would be necessary to append other pluto-specific comments later.

[lungben]

🛑 is a unicode symbol, it seems that your editor or font does not support it (it works for me in VSCode).
However, it is trivial to change the symbol - it this version working better for you?

[ederag]

However, it is trivial to change the symbol - it this version working better for you?

With a94685f, the symbols are visible, thanks !

# ╔═╡ d5c3ed28-8854-4381-bcf3-9f764af94643
#= ╠═ disabled ═╣
sol = solve(prob, Tsit5(), reltol=1e-8, abstol=1e-8)
╠═ disabled ═╣ =#

Just in case it would be an overlook, I find it a bit cleaner with the tee aligned vertically
(with ╠═╡ and no closing symbol, for consistency with other Pluto comments),

# ╔═╡ d5c3ed28-8854-4381-bcf3-9f764af94643
#=╠═╡ disabled
sol = solve(prob, Tsit5(), reltol=1e-8, abstol=1e-8)
  ╠═╡ disabled =#

Otherwise, disable/re-enable dependent cells works as expected.

Good: if someone mess with the comments in the file, by inadvertence,
in Pluto the code just appears inside an enabled cell, together with the messed-up comments, easy to remove,
no loss of information.

Up to now it looks robust 🙂.

[lungben]

Thanks, is corrected.

[ederag]

In the file, there are two empty lines after a disabled cell, but only one for enabled cells.
Removing one of these empty lines in the file, and reopening in Pluto leads to syntax error:
the last character of the valid code has been lost.

Current PR works, but it might be nice to have a consistent layout,
since editing is sometimes used as a workaround, and there are some Pluto files producers in the works (e.g. #132) ?

While at it: why not include the trailing \n in _running_disabled_prefix ?

[lungben]

Good catch, is corrected!

[lungben]

@fonsp from my side this PR is ready for merging now.
Or do you have any further suggestions?

[fonsp]

Thanks! Since this is a (non-breaking) file change, I would like to wait a bit before merging. I would also like to see more feedback on the disabling feature.

One thought is that we might need to better communicate "disabled cells" when opening a notebook, since the feature will now be exposed to people who have never used Pluto before and opened someone else's file.

[lungben]

Maybe show a popup window, similar to the undo window for cell deletion, when at least one cell is deactivated?
This popup window should give the option to reactivate all cells.

A bit off-topic, but a keyboard shortcut for toggeling the current cell between enabled and disabled would be helpful, too.

[lungben]

Another small point: currently, if a cell with a higher cell_precedence_heuristic (e.g. a cell containing a using statement) is deactivated, dependent cells are not indirectly deactivated because the dependencies are not known.
One way to deal with it would be to deactivate all cells with higher precedence if a cell with non-default (highest) precedence is deactivated.

[ederag]

That's a valid concern, but I'd rather disable only cells that are known for sure to error due to the disable,
and wouldn't risk to deactivate cells that are actually independent on this "using" cell.
The cells really depending on the "using" cell would just error with a meaningful message, so it looks good enough for now ?

[lungben]

Maybe show a popup window, similar to the undo window for cell deletion, when at least one cell is deactivated?
This popup window should give the option to reactivate all cells.

A bit off-topic, but a keyboard shortcut for toggeling the current cell between enabled and disabled would be helpful, too.

For these parts I would need help from a JS expert.
Should we put these points in this PR or rather in a separate one?

[ederag]

Maybe show a popup window, similar to the undo window for cell deletion, when at least one cell is deactivated?
This popup window should give the option to reactivate all cells.

Those popups are perhaps useful for deletion that otherwise would be irreversible,
but I find them a bit distracting. And disable is easily reversible.
Not strong opinion here, just unsure it is worth the time.

A bit off-topic, but a keyboard shortcut for toggeling the current cell between enabled and disabled would be helpful, too.
Should we put these points in this PR or rather in a separate one?

Would be nice, but quite separate indeed.

[fonsp]

I think we should:

• move the depends_on_disabled_cells field from the Cell struct to the CellDependencies struct (update other code to match)
• move the calculation of that field to update_dependency_cache!
• leave the Run.jl file mostly unchanged, computing the field twice is not a problem. Keeping Run.jl simple is more important
• add a test for indirect deps using the jl_file_is_runnable function in test/helpers.jl

[lungben]

Done

[pankgeorg]

@pankgeorg

• Keyboard shortcut to enable/disable cells
• Some GUI to enabled all disabled cells at one

[ederag]

@pankgeorg
do you mean they prevent merging this PR (as the checkboxes imply),
or are they good ideas that came to mind while reviewing ?

[pankgeorg]

it's very nice to have @ederag. let's discuss it next Thursday on the pluto devs call !! - see here!

[dralletje]

Heyyy, I have another request here: Can we wait with this until we have a more generic way to store extra metadata with cells?

OR???

Let a disabled cell be a cell that is commented out fully?
Just like in the Chrome style explorer, when you disabled a style with the checkbox, it "just" comments it (if you comment a style out, it will show up in the style explorer, but unchecked).
With that we also have shortcuts figured:

Keyboard shortcut to enable/disable cells

Inside cell: Control a + Control /
With cell selected: Control /

Some GUI to enabled all disabled cells at one

Not focussed on a cell: Control a + Control /

Pros:

• it's awesome

Cons:

• Dependent disabled cells can't be commented, sadly, but
  • We can have a metadata field for these anyway OR
  • We think of something smart that would make storing this not necessary at all?
• Pasting commented code or just commenting your code will possibly have side effects your don't intend..
  • Would also need a smart solution

Now I'm looking over it I might make this a bit more complex than it need to be... Maybe merge this first (with proper TOML-cell-config) and then look into something more fancy

[ederag]

Let a disabled cell be a cell that is commented out fully?

That is already the case with this PR, in the .jl file

# ╔═╡ d5c3ed28-8854-4381-bcf3-9f764af94643
#=╠═╡ disabled
sol = solve(prob, Tsit5(), reltol=1e-8, abstol=1e-8)
  ╠═╡ disabled =#

But I find it good that it is only dimmed out in the Pluto display,
and not simply commented out,
since the last valid output is shown (dimmed out also).
It feels more consistent, preserves more information,
and makes it easier to locate the right cell to reenable, later.

Conversely, if you merely comment out the cell content, as I understand your suggestion, then

1. the syntax highlighting is lost
2. the output is lost
3. there is no way for Pluto to tell the difference between a disabled cell (whose children are disabled)
   and a commented out cell (whose children should error), so the metadata you talk about is needed anyway.

Reading again, it looks like you figured that out in the end, so please take this only as a clarification for others.

[dralletje]

That is already the case with this PR, in the .jl file

I'm so not used to seeing #=, hahaha.
I was thinking prepending all the lines with # but this works just as well.
Isn't prepending every line with # a bit more error-prone? Now we also need to check cells for not having a dangling =# D:

But I find it good that it is only dimmed out in the Pluto display,
and not simply commented out,

Yeah!! Visually, very much!! I'm thinking more the other way, so when you select a whole cell and comment it out, it becomes disabled and dimmed. I like the dimmed version way better than this huge pink block of comments :P

Reading again, it looks like you figured that out in the end, so please take this only as a clarification for others.

Yeah... 😅 Thanks!
So what about commenting, but storing the disabled = true (and disabled = dependent or something) using #1482 (If I get this is in before the end of the week 😁)?
So it'd be like

# ╔═╡ d5c3ed28-8854-4381-bcf3-9f764af94643
# ╠═╡ disabled = true
#=
sol = solve(prob, Tsit5(), reltol=1e-8, abstol=1e-8)
=#

or

# ╔═╡ d5c3ed28-8854-4381-bcf3-9f764af94643
# ╠═╡ disabled = true
# sol = solve(prob, Tsit5(), reltol=1e-8, abstol=1e-8)

[lungben]

Another cool emergent feature of cell disabling is that you can use variables defined in a disabled cell in a new cell without an error, the new cell gets automatically disabled.

I like the idea of using TOML meta-data!

Regarding commenting out by line (with #) or block-wise (with #=) I do not have a strong preference. I originally used block-wise because it was easier this way to distinguish between "normal" comments and disabled cells (using the metadata after the block comment sign), but with the TOML meta-data this would not be an issue anymore.

[postylem]

Where does this PR stand in terms of getting merged? I'm working on using Pluto to create assignments and being able to disable cells which the students must modify before they will work, or cells which will run for an extremely long time is a really nice feature I was hoping to use. I didn't realize the disabledness state doesn't currently get saved.

[lungben]

The issue is that this PR would change/enhance the Pluto file format, which is not reversible (however, the change is backward-compatible).

Thus, if #1482 should be introduced, it is better to do it before this PR and store the cell disabling status in that new metadata.
@dralletje what is the status of that PR?

[ederag]

Since this is stalled anyway, here is an answer to #1209 (comment):

Prepending # on every lines is simpler,
but leads to much larger diffs.
With block comments, cell disable is a two lines change
with clear purpose.

With the nice toml idea #421 or #1482 it would be tempting to add a # ╟─ disabled = true line,
but that would either

• [prepend strategy] save a single line in the source, but add a lot of + # ... and - # ... lines in the diffs, or
• [block quote strategy] add another line in the source
  (no big deal, but labelled block quotes make it clear which comments are due to Pluto).

Yet it's true that executing a .jl with dangling #= would syntax error as
incomplete: unterminated multi-line comment #= ... =#,
and a .jl with a dangling =# would syntax error as
unexpected "=".
Neither affect Pluto, and both are easy to fix in the source if needed, so it does not look so bad to me ?

[ederag]

it is better to do it before this PR and store the cell disabling status in that new metadata.

@lungben For the "prepend # on each line" strategy sure,
but with the block quote strategy (your PR, which I like 🙂), those PR could be well separated.

[lungben]

How to proceed here:

1. First implement the cell metadata functionality !1482 (@fonsp already started with it, I can continue)
2. Then reimplement this PR based on the new Pluto main and Way from julia and javascript to store metadata in notebook file #1482 (there have been so many changes in Pluto that it is easier to re-implement this PR than merging main into this PR).

[lungben]

This functionality is implemented in #1895, closing this PR now.