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

Managing notebook metadata #5968

Merged
merged 71 commits into from Apr 6, 2019
Merged

Conversation

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Feb 8, 2019

Fixes #5200 and #5235

Here are a few user stories:

Jane opens and edits a notebook, and wants to save it back a single time in a 'finished' state, meaning she wants to capture all of the view information in the notebook document, such as collapse state, widget state, etc. She doesn't typically want to save this information.

Tom regularly wants to save a notebook without any outputs, in a minimal state, with no extra information. He'd like to do this from a single keyboard shortcut, preferably as the default save action.

Mary opens a notebook that has initial view state (such as collapsed/scrolled cells, etc.). She works in the notebook, changing this view state, running new cells, etc. She'd like to save her code and output changes, but not change the original initial view state (e.g., she's expanded some cells, edited them, but she wants the saved notebook to reflect the original collapsed state, whatever it was.)

Jake has been working in a notebook, and would like to save off a copy in a different file with all outputs deleted, but with the cell view state as he currently has it. He doesn't want to modify his current file.

To try out this branch on binder, follow this link: Binder

TODO

  • Pull in whatever makes sense from #6009 (tests, for example?)
  • Put back in convenience things in the notebook tracker, like current cell, etc.
  • UX review
@jasongrout jasongrout added this to the 1.0 milestone Feb 8, 2019
The dialog definitely needs some UX work.
@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Feb 11, 2019

Here's the current status:
screen shot 2019-02-11 at 9 30 32 am
screen shot 2019-02-11 at 9 28 14 am

The basic idea is that an extension can register a 'save option', along with label/help text and a default choice for the action. The action (what the user can choose in a dropdown) is one of:

  • save - Save the current state to the notebook model
  • clear - Clear the state from the notebook model
  • previous - leave the current state of the model alone.

The default choices are given by the setting system, or if there is no setting for the option name, the default action provided in the option registration. The 'remember my choices' checkbox records the names and values to the user settings for the notebook.

Here's the basic idea, from the perspective of an extension developer:

  tracker.saveOptions.push({
    name: 'collapse-scroll-state',
    label: 'Cell collapsed and scrolled state',
    help:
      'The input and output collapsed state, as well as the output scrolled state for code cells',
    action: 'save',
    callback: (w: Notebook, name: string, action: SaveAction) => {
      switch (action) {
        case 'clear':
          NotebookActions.clearCollapseScrollState(w);
          break;

        case 'save':
          NotebookActions.persistCollapseScrollState(w);
          break;

        case 'previous':
        default:
      }
    }
  });

A number of questions come up:

  1. I don't think there is a lot of precedent for having two save menu items. It seems that usually there is a single 'save', then it is 'export' or 'save as' that exposes options for saving. Should we follow a pattern like that?
  2. For some options, like the view state, or the widget state, where the optional data is some initial state that isn't changed in course of running the notebook, all three options make sense, i.e., clear the state in the model, save the current state to be the new initial state, or just leave the initial state in the model alone. However, for other options such as whether to clear outputs, which is being changed continuously as the notebook is used, previous doesn't really make sense, since we don't keep the original outputs. For clearing outputs, it makes more sense to have a checkbox, i.e., either clear the outputs or not.
  3. Is it surprising that changing an option, such as 'cell outputs', actually changes the notebook drastically (i.e., clears outputs)?
  4. If we 'remember my choices', should that affect the default save (without the dialog)?

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Feb 11, 2019

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Feb 11, 2019

@ian-r-rose will know better, but I think the menu option should be capitalized as "Save Notebook with Options..."

jasongrout added 2 commits Feb 11, 2019
The inline form groups don’t look very good to me for this dialog.

(Also, commit some linting fixes.)
@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Feb 11, 2019

will know better, but I think the menu option should be capitalized as "Save Notebook with Options..."

Fixed.

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Feb 11, 2019

Also dropdown options should be capitalized in title case.

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Feb 11, 2019

Also dropdown options should be capitalized in title case.

Thanks for the small corrections. The much bigger elephants in the room remain... :)

@afshin
Copy link
Member

@afshin afshin commented Feb 11, 2019

I think one common pattern is:

  • "Save"
  • "Save As..." where this brings up a dialog that is collapsed by default and allows you to add a file name but has a caret that can be expanded with a bunch of options.

What do you think of employing this UX pattern?

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Feb 11, 2019

What do you think of employing this UX pattern?

That is where I was going with my question 1, asking what others thought :). I think one common use case is people actually saving to the same file with various options. This would be easier if the save as dialog defaulted to saving over the current file, though that also might be confusing.

@afshin
Copy link
Member

@afshin afshin commented Feb 11, 2019

I just took a quick look at my editors and the three I use most have multiple "Save" options. They all have "Save" and "Save As..." and in all three cases, the "Save As..." is collapsed but can be expanded. I think I've grown accustomed to this pattern.

  • Sublime Text
    screen shot 2019-02-11 at 18 20 18
  • Visual Studio Code
    screen shot 2019-02-11 at 18 16 01
  • TextMate
    screen shot 2019-02-11 at 18 16 25

And here is a collapsed "Save As..."
screen shot 2019-02-11 at 18 16 45
Compared with an expanded "Save As..."
screen shot 2019-02-11 at 18 19 54

@afshin
Copy link
Member

@afshin afshin commented Feb 11, 2019

It's not a perfect analogy because our options are more specific than just showing a nice file browser in the dialog, but I think it's good as prior art.

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Feb 11, 2019

Another precedent: macOS Preview "Export" option, which does default to overwriting the current file:
screen shot 2019-02-11 at 10 26 00 am

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Feb 11, 2019

Another precedent: Word on macOS "Save As...", again defaults to overwriting the current file. The options button here pulls up an application-wide generic "saving options" button, which isn't about setting options for this save, but about options for how word saves things.
screen shot 2019-02-11 at 10 27 45 am
Here is the html "save as..." that shows some save-specific options:
screen shot 2019-02-11 at 10 28 34 am

@afshin
Copy link
Member

@afshin afshin commented Feb 11, 2019

Yeah, I agree that defaulting to overwriting is reasonable.

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Feb 11, 2019

I think you can also simplify and align the dialog controls. I am thinking about something that has labels right aligned and the controls left aligned. This is described in Apple's HIG here:

https://developer.apple.com/design/human-interface-guidelines/macos/fields-and-labels/labels/

I think it would make sense for us to create a Label react component to encode this pattern.

The only question is where to put the longer descriptions. from the HIG I think the best place would be under the control, L aligned with the control above. The description you have for the cell output is redundant with the Label.

Finally, I think we are using a smaller dropdown size in the UI typically (see the toolbar usage).

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Feb 11, 2019

And Labels should be title case.

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Feb 11, 2019

@jasongrout let me know if you want me to post a mockup of the visual design feedback.

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Feb 11, 2019

@ellisonbg - thanks again for a detailed review of the dialog. First, I think it makes sense to address the architecture questions listed above, such as whether we should even have a separate "save with options" dialog at all, or whether it should be folded into a Save As... dialog. Second, do the three options presented make sense, or should we have flexibility for checkboxes (like for cell outputs).

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Mar 30, 2019

Perhaps we should just convert the notebook cell metadata once when it is initially read from disk. Then the runtime behavior isn't confusing, the transformation is just done once when the data comes from disk.

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Apr 1, 2019

Okay, one final change here - at cell model creation, we make sure that collapsed and jupyter.outputs_hidden are synced (looking at collapsed, then jupyter.outputs_hidden in that order). From then on, at the cell model level, we make sure that these two pieces of metadata are synced.

This makes sure that these redundant pieces of metadata always agree.

See jupyter/nbformat#137
@jasongrout jasongrout force-pushed the save-with-options branch from b6d71f0 to 36dfc3e Apr 1, 2019
@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Apr 1, 2019

All checks passed! Thanks @afshin and @ian-r-rose and @saulshanabrook for your work on getting the test suite to pass!

Copy link
Member

@afshin afshin left a comment

(I am still working my way through the code.)

packages/apputils/src/collapse.ts Outdated Show resolved Hide resolved
packages/apputils/src/collapse.ts Outdated Show resolved Hide resolved
packages/apputils/src/collapse.ts Show resolved Hide resolved
packages/apputils/src/instancetracker.ts Show resolved Hide resolved
packages/docmanager-extension/src/index.ts Show resolved Hide resolved
@tgeorgeux
Copy link
Contributor

@tgeorgeux tgeorgeux commented Apr 4, 2019

@jasongrout this is super nit-pickey, but I think if we could solve it now it would help in the future as well.

I think the amount of padding or margin on the bottom should match the top. I tried to mess around with the CSS in the inspector, but I caused more problems than I fixed.
Margin or padding

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Apr 4, 2019

@tgeorgeux - thanks. Can you indicate on the image exactly what spaces should be equal?

@tgeorgeux
Copy link
Contributor

@tgeorgeux tgeorgeux commented Apr 4, 2019

@jasongrout yes, sorry that picture wasn't as clear as I originally thought.

more precise

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Apr 5, 2019

@tgeorgeux I got the padding close:
Screen Shot 2019-04-05 at 3 24 32 PM

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Apr 5, 2019

@afshin - it works better now with f1830f4 :
screenshot

afshin
afshin approved these changes Apr 5, 2019
Copy link
Member

@afshin afshin left a comment

Thanks for the massive amount of work and back-and-forth, @jasongrout!

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Apr 6, 2019

Thanks! Will you merge it?

@afshin afshin merged commit 1bd8d78 into jupyterlab:master Apr 6, 2019
9 checks passed
@afshin
Copy link
Member

@afshin afshin commented Apr 6, 2019

Boom!

@meeseeksmachine
Copy link
Contributor

@meeseeksmachine meeseeksmachine commented Apr 10, 2019

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

https://discourse.jupyter.org/t/could-not-find-model-due-to-outputs-text-plain-truncated-at-111-bytes/733/5

@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/5

@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.

9 participants