Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Persist code cell collapsed #3981

Merged
merged 18 commits into from May 28, 2018
Merged

Conversation

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Feb 23, 2018

This adds support for loading the collapsed state of a code cell output from the collapsed metadata property, as well as saving to that property when the persistOutputsCollapsed command is run.

We do this by just storing the persisted value of the collapsed state in the CodeCellModel. The view initializes itself from this value, but when it is updated (you collapse a cell) it doesn't update the model. Only when the command is explicitly run does it set the metadata value in the model, which is then saved to disk when it is serialized to JSON.

This addresses the collapsed output cells part of #3799 but doesn't work on persisting the input cells.

  • load collapsed state to model from file
  • load view state from model
  • Add command to copy from view to model
  • Add menu item as well
@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented Feb 26, 2018

@ellisonbg Let me know if this is implemented how you envisioned.

I don't find it particularly intuitive to have to run persistOutputsCollapsed to save the collapsed states, but it does the job of keeping that state independent of the file on disk.

As far as I understand it, this supports the case where two people have the same file open at once in different JupterLab instances and want to have different things collapsed. However, if this isn't a case we want to support, than just having the collapsed state part of the model makes more sense.

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Mar 7, 2018

Things we talked about today in the meeting:

  • Keep the command to manually persist the state.
  • Add a new extension point and keyboard shortcut (command+shift+S) that extensions can do additional things with before saving (such as persist this state).

@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented Mar 7, 2018

Add a new extension point and keyboard shortcut (command+shift+S) that extensions can do additional things with before saving (such as persist this state).

Is there anything else at the moment that should also be saved here?

Here are a couple of options I was thinking for the label:

  • Save with View
  • Save with Current View
  • Save with View State

Or, if we want to keep it collapsed state specific atm

  • Save Notebook with Collapsed State
  • Save Notebook with Collapsed/Revealed

/cc @afshin

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Mar 7, 2018

@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented Mar 8, 2018

I added an extension point to the file menu that is similar to the "close and clean" one. It let's any widget register a custom save command, that has some extra logic. If it isn't implemented, it will appear as "Save with Extras":

screen shot 2018-03-08 at 11 10 31 am

For the notebook, it says "Save Notebook with View State":
screen shot 2018-03-08 at 11 10 44 am

I bound this by default to CMD SHIFT S.

@jasongrout jasongrout added this to the Beta 3 milestone Mar 28, 2018
@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 17, 2018

I am glad to see the menu extension system getting some use! Looks like it needs a rebase, but the overall implementation looks sound to me.

My main question is this: will this functionality be widely-used enough that initiallyCollapsed should exist at the top-level of the cell model? As far as I can tell, we could accomplish the same thing by directly editing the cell metadata for 'collapsed', rather than another layer of indirection in the ICellModel.initiallyCollapsed.

Put another way, I think there is value to having the cell model be reasonably close to that described in nbformat.

@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented May 22, 2018

@ian-r-rose thanks for looking over this.

As far as I can tell, we could accomplish the same thing by directly editing the cell metadata for 'collapsed', rather than another layer of indirection in the ICellModel.initiallyCollapsed.

Yeah it took my like ten looking over my code to figure out what it was doing, so it should be changed somehow. Also, I don't why I added that indirection. I will try rebasing and removing that and hopefully that will be clearer and still work properly.

@saulshanabrook saulshanabrook force-pushed the persist-collapsed branch 2 times, most recently from 8a91e83 to 2cec23b May 22, 2018
@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented May 22, 2018

@ellisonbg Coming back to this, I wonder if the notebook specific save should be 'Save Notebook with Collapsed State' instead of 'Save Notebook with View State'. Otherwise, the user will not really know what is part of the view state. If we extend the command in the future to do more, we could update the name to be more general.

@ian-r-rose I removed the extra abstraction, should be more readable now.

Copy link
Member

@ian-r-rose ian-r-rose left a comment

Just took this for a spin, and it works really nicely. Am I right in thinking that "collapsed" is a semi-official metadata field for this behavior? In the current UI both inputs and outputs are able to be collapsed, but this only applies to outputs, so I found the naming to be a touch confusing. I don't think it's a big problem, though.

persistAndSave: (current: NotebookPanel) => {
NotebookActions.persistOutputsCollapsed(current.notebook);
app.commands.execute('docmanager:save');
return Promise.resolve();
Copy link
Member

@ian-r-rose ian-r-rose May 22, 2018

Is there a reason you are returning a resolved promise here, rather than return app.comands.execute('docmanager:save');?

Copy link
Member Author

@saulshanabrook saulshanabrook May 22, 2018

Nope. I have changed it.

*/
export
function persistOutputsCollapsed(widget: Notebook): void {
if (!widget.model || !widget.activeCell) {
Copy link
Member

@ian-r-rose ian-r-rose May 22, 2018

Do we need an active cell check here? As far as I know, we should always have one, but it doesn't seem strictly necessary for the functionality.

Copy link
Member Author

@saulshanabrook saulshanabrook May 22, 2018

Nope I don't believe we do, I was just copy and pasting. Removed.

"default": { },
"properties": {
"command": { "default": "filemenu:persist-and-save" },
"keys": { "default": ["Accel Shift S"] },
Copy link
Member

@ian-r-rose ian-r-rose May 22, 2018

This now conflicts with the "Save As..." shortcut (sorry, I know your PR predates that!). There is a lot of precedence for "Save As..." having Accel Shift S as a shortcut. Can we choose something else?

Copy link
Member Author

@saulshanabrook saulshanabrook May 22, 2018

I changed to Accel Ctrl S which works for me on mac/chrome.

NotebookActions.persistOutputsCollapsed(widget);
for (const cell of widget.widgets) {
if (cell instanceof CodeCell) {
expect(cell.model.initiallyCollapsed).to.be(cell.outputHidden);
Copy link
Member

@ian-r-rose ian-r-rose May 22, 2018

This looks like it needs to be fixed.

Copy link
Member Author

@saulshanabrook saulshanabrook May 22, 2018

Yep. Changed to use metadata.

Copy link
Member

@ian-r-rose ian-r-rose left a comment

Thanks! This looks good to me, pending CI. If we want to change the label to "Collapsed State", that's cool too.

@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented May 22, 2018

Am I right in thinking that "collapsed" is a semi-official metadata field for this behavior?

Yeah it is. AFAIK in the notebook we just have a single collapsed metadata for the whole cell. I don't know how to specify that the input collapsed vs the output is collapsed:

screen shot 2018-05-22 at 2 26 52 pm

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 22, 2018

Does the classic notebook collapse the whole cell, or just the outputs?

Edit: looks to me like just the outputs. In which case, I think you have the correct behavior (even if the naming is a bit confusing).

@akhmerov
Copy link
Member

@akhmerov akhmerov commented May 22, 2018

There's a separate outputs_hidden

EDIT: also a separate source_hidden a couple of lines above.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 22, 2018

As far as I can tell, outputs_hidden and collapsed have very similar behavior.

@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented May 22, 2018

Looks like collapsed maybe should enabling scrolling for output and outputs_hidden and inputs_hidden should control if they are actually hidden (src, nbformat discussion).

So I guess I should update the PR to use outputs_hidden and maybe also include support for inputs_hidden.

EDIT: Does that seem like the right approach?

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 22, 2018

Thanks for the link @akhmerov. I don't see a difference in how the classic notebook handles collapsed vs outputs_hidden, nor do I see any effect for inputs_hidden. Is it possible that the classic notebook doesn't implement this feature?

@akhmerov
Copy link
Member

@akhmerov akhmerov commented May 22, 2018

AFAIR the classic notebook indeed never made use of these fields.

@akhmerov
Copy link
Member

@akhmerov akhmerov commented May 22, 2018

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 22, 2018

In that case, I see a couple of options:

  1. Use collapsed as an alias for jupyter.outputs_hidden. Also introduce jupyter.inputs_hidden in the same fashion as this PR.
  2. Use jupyter.inputs_hidden and jupyter.outputs_hidden as intended. Use collapsed as an alias for jupyter.inputs_hidden && jupyter.outputs_hidden.

Thoughts?

@akhmerov
Copy link
Member

@akhmerov akhmerov commented May 22, 2018

Should collapsed be used in this context at all? I believe it's older and has a different use.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 22, 2018

I agree that it's older, but I was hoping to avoid breaking existing notebooks, and it is in the schema. Can you elaborate on what the different use is? Again, I could not see what it did other than hide the outputs.

@akhmerov
Copy link
Member

@akhmerov akhmerov commented May 22, 2018

Can you elaborate on what the different use is?

I was perhaps too hasty. I base my evaluation on the issue discussion found by @saulshanabrook (the nbformat docs are also not very helpful BTW). The discussion suggests that collapsed is perhaps more like "visible but occupying very little space". The current jupyterlab UI does not have an alternative means of showing such state, instead only implementing something more similar to output hiding.

Based on the above I believe that:

  • Jupyterlab should render cells that are collapsed the same way as cells with jupyter.outputs_hidden. In other words, jupyterlab shouldn't render outputs if collapsed || jupyter.outputs_hidden.
  • Whether the input is not shown should not depend on whether the cell is collapsed due to compatibility with the classic notebook.
  • Finally, it is probably a matter of choice whether hiding output using the jupyterlab UI should set the collapsed metadata. Given that right now classic notebook is extremely popular, I believe both flags should be set whenever used hides an output from jupyterlab UI.

@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented May 22, 2018

@akhmerov Thanks for the suggestions.

The discussion suggests that collapsed is perhaps more like "visible but occupying very little space". The current jupyterlab UI does not have an alternative means of showing such state, instead only implementing something more similar to output hiding.

Do you think enabling scrolling for outputs covers this use case? Since it limits how much vertical space the outputs take up?

@akhmerov
Copy link
Member

@akhmerov akhmerov commented May 22, 2018

Do you think enabling scrolling for outputs covers this use case? Since it limits how much vertical space the outputs take up?

To be honest, I'm not an ultimate nbformat authority, and the docs are not too specific. Let's try to see WWCND (what would classic notebook do).

  1. Pristine cell
    image
  2. "scrolled": true
    image
  3. "collapsed": true
    image

So it seems it is would be more in line with the classic notebook to unify collapsed with jupyter.outputs_hidden than with scrolled.

@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented May 28, 2018

I rebased and updated the current.notebook locations.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 28, 2018

Great, thanks for all the back and forth!

@ian-r-rose ian-r-rose merged commit 3202ec5 into jupyterlab:master May 28, 2018
2 checks passed
@saulshanabrook saulshanabrook deleted the persist-collapsed branch May 28, 2018
@dclong
Copy link

@dclong dclong commented Jun 30, 2018

Does nbconvert respect folded cells? That is when we export the notebook to an HTML, will folded cells in the notebook be folded in the HTML?

@akhmerov
Copy link
Member

@akhmerov akhmerov commented Jul 1, 2018

@dclong quoting the changelog of nbformat:

Introduce source_hidden and outputs_hidden as official front-end metadata fields to indicate hiding source and outputs areas. NB: These fields should not be used to hide elements in exported formats.

If I remember correctly though, modifying nbconvert config to use these fields should be relatively straightforward.

@meeseeksmachine
Copy link
Contributor

@meeseeksmachine meeseeksmachine commented Jul 25, 2019

This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there:

https://discourse.jupyter.org/t/hiding-code-cell-on-launch/1763/4

@lock lock bot locked as resolved and limited conversation to collaborators Aug 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants