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 header and toolbar toggle status in nbconfig #1769

Merged
merged 4 commits into from Oct 6, 2016

Conversation

@gnestor
Copy link
Contributor

@gnestor gnestor commented Sep 16, 2016

A first stab at #1661

If `notebook.header` or `notebook.toolbar` are `false`, hide them on
load
@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Sep 16, 2016

@Carreau I am following your lead from c040464. However, neither the header or toolbar properties are being persisted in notebook.config.data after refresh. I don't fully understand how nbconfig works so I would appreciate your input here (I've enabled "Allow edits from maintainers" so you should be able to push to this PR).

@gnestor gnestor added this to the 5.0 milestone Sep 16, 2016
@Carreau
Copy link
Member

@Carreau Carreau commented Sep 19, 2016

Yeah, nbconfig thing is complex. let me have a look. Thanks for that.

@Carreau
Copy link
Member

@Carreau Carreau commented Sep 19, 2016

So it seem you get the config right, the problem is the asynchronicity and the config return defaults before being completly loaded.

if you cat ~/.jupyter/nbconfig/notebook.json you can see the values get changed.

You might want to add a this.config.loaded.then(function(){stuff(that.config.data)}

The resolve part of the promise does not get passed the new config apparently, but that should be fixable in notebook/services/config.js around line 34:

 ConfigSection.prototype._load_done = function() {
        if (!this._one_load_finished) {
            this._one_load_finished = true;
            this._finish_firstload();  // might want to pass self.data here. 
        }
    };

Does that make sens ?

@takluyver
Copy link
Member

@takluyver takluyver commented Sep 19, 2016

The idea with the config is that you can create a ConfigWithDefaults object, and then you have a choice of:

// Get a value immediately, using the defaults if real values aren't loaded yet
visible = c.get_sync('toolbar_visible');

// Wait for the value if necessary
c.get('toolbar_visible').then(function(visible) {...});
@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Sep 20, 2016

@Carreau Your this.config.loaded.then() suggestion worked great...

Two questions:

  1. Is there a place to define default config values so that I don't have to wonder if this.config.data.Header is defined or not?
  2. Is there a better place to check the config values and hide/show the header/toolbar than in the initialization of the Notebook class (e.g. Notebook.load_notebook)?
@takluyver
Copy link
Member

@takluyver takluyver commented Sep 20, 2016

Is there a place to define default config values so that I don't have to wonder if this.config.data.Header is defined or not?

There's a ConfigWithDefaults class, used like this:

configwd = ConfigWithDefaults(this.config, {
    header: true,
    toolbar: true,
});
configwd.get('header').then(function(header) {
    if (!header) {
        $('#header-container').hide();
        ...
    }
});
Move toggle logic to `notebook.js` vs. `actions.js` to be consistent
with `notebook.line_numbers`
@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Sep 20, 2016

Thanks @takluyver! I think this is ready for review...

Copy link
Contributor Author

@gnestor gnestor left a comment

Let's give the Review feature a try...

}
});

this.class_config.get('Header').then(function(header) {

This comment has been minimized.

@gnestor

gnestor Sep 20, 2016
Author Contributor

To revisit my other question: Is there a better place to put this (like Notebook.load_notebook)?


Object.defineProperty(this, 'header', {
get: function() {
return that.class_config.get_sync('Header');

This comment has been minimized.

@gnestor

gnestor Sep 20, 2016
Author Contributor

I switched from ConfigSection to ConfigWithDefaults to take advantage of defaults.

handler : function(env){
$('div#maintoolbar').toggle();

This comment has been minimized.

@gnestor

gnestor Sep 20, 2016
Author Contributor

I also moved header/toolbar toggle logic from actions.js to notebook.js to be consistent with the line numbers toggle implementation.

}

})
get: function() {

This comment has been minimized.

@gnestor

gnestor Sep 20, 2016
Author Contributor

I tried to update the line numbers toggle logic to use ConfigWithDefaults (via that.class_config.get_sync and that.class_config.set) to be consistent with header and toolbar but it looks like the line numbers toggle status is being read from notebook.config.data regardless. notebook.json looks like:

{
  "Notebook": {
    "Toolbar": false,
    "Cell": {
      "cm_config": {
        "lineNumbers": false
      }
    },
    "Header": false
  },
  "Cell": {
    "cm_config": {
      "lineNumbers": true
    }
  }
}

The top-level Cell is set using notebook.config.update while Notebook.Cell is set using notebook.class_config.set. Changing the value of Notebook.Cell.cm_config.lineNumbers has no effect even after changing the line numbers toggle logic, so the cells/Codemirror appear to be accessing Cell.cm_config.lineNumbers directly vs. accessing notebook.lineNumbers. I know this is probably confusing and that's because it is! It doesn't really matter because everything is working but it would be nice if these 3 properties shared the same implementation.

@@ -462,38 +462,23 @@ define(function(require){
env.notebook.show_command_palette();
}
},
'show-all-line-numbers': {

This comment has been minimized.

@takluyver

takluyver Sep 21, 2016
Member

Actions are quite public API - they're used to define keyboard shortcuts - so I'd rather not remove them without a good reason.

This comment has been minimized.

@gnestor

gnestor Sep 21, 2016
Author Contributor

Good point. I added back the show/hide line numbers as well as adding show/hide actions for header and toolbar.


this.class_config.get('Header').then(function(header) {
if (header === false) {
that.header = false;

This comment has been minimized.

@takluyver

takluyver Sep 21, 2016
Member

This feels a bit icky - it waits for the config to load, and then does something which sets the value from the config back into the config.

I think there should be a separate piece of API which actually hides/shows the toolbar on the page, and a higher level wrapper around that which also pushes the state into config.

This comment has been minimized.

@gnestor

gnestor Sep 21, 2016
Author Contributor

This is a good point, thanks for the feedback. I just pushed a new commit that moves the toggle logic to the actions and then calls the actions to toggle the header/toolbar on notebook load.

@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Sep 22, 2016

@Carreau Would you care to review? It's working for me and I think it's quite a bit cleaner given @takluyver's feedback. Two questions for you:

  1. I would like notebook.line_numbers to use ConfigWithDefaults as well to keep these properties consistent. I tried it and the issue is this: ConfigWithDefaults (or notebook.class_config.set) saves the line numbers toggle state at Notebook.Cell.cm_config.lineNumbers and ConfigSection (or notebook.config.update) saves it at Cell.cm_config.lineNumbers. After switching to ConfigWithDefaults, the cells no longer reflected the state in Notebook.Cell.cm_config.lineNumbers after refresh. However, if I manually changed the value of Cell.cm_config.lineNumbers in notebook.json, then they would reflect that. Sooo, the cells are not accessing the line numbers toggle state from notebook.line_numbers but appear to accessing it via notebook.config.data.Cell.cm_config.lineNumbers. My question for you: where does Codemirror look to determine whether a cell should display line numbers or not (specifically on load)?
  2. Is there a better place to put the "check nbconfig for header/toolbar toggle state and hide/show them accordingly" behavior than in the initialization of the notebook class (like notebook.load_notebook for example)?
@Carreau
Copy link
Member

@Carreau Carreau commented Sep 23, 2016

I'm giving a seminar today and will talk to a lot of people before and
after so won't be online a lot. I'll have a look ASAP.

On Thu, Sep 22, 2016 at 12:54 PM, Grant Nestor notifications@github.com
wrote:

@Carreau https://github.com/Carreau Would you care to review? It's
working for me and I think it's quite a bit cleaner given @takluyver
https://github.com/takluyver's feedback. Two questions for you:

  1. I would like notebook.line_numbers to use ConfigWithDefaults as
    well to keep these properties consistent. I tried it and the issue is this:
    ConfigWithDefaults (or notebook.class_config.set) saves the line
    numbers toggle state at Notebook.Cell.cm_config.lineNumbers and
    ConfigSection (or notebook.config.update) saves it at
    Cell.cm_config.lineNumbers. After switching to ConfigWithDefaults, the
    cells no longer reflected the state in Notebook.Cell.cm_config.
    lineNumbers after refresh. However, if I manually changed the value of
    Cell.cm_config.lineNumbers in notebook.json, then they would reflect
    that. Sooo, the cells are not accessing the line numbers toggle state
    from notebook.line_numbers but appear to accessing it via
    notebook.config.data.Cell.cm_config.lineNumbers. My question for you: where
    does Codemirror look to determine whether a cell should display line
    numbers or not (specifically on load)?
  2. Is there a better place to put the "check nbconfig for
    header/toolbar toggle state and hide/show them accordingly" behavior than
    in the initialization of the notebook class (like
    notebook.load_notebook for example)?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1769 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAUez2t4sDZ71KaM56Al57PNjhchdlrxks5qstzogaJpZM4J_FhF
.

@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Sep 23, 2016

@Carreau Ok no problem!

@Carreau
Copy link
Member

@Carreau Carreau commented Sep 23, 2016

Notebook.Cell.cm_config.lineNumbers

this is due to the fact that line number could be toggle per-cell.
I think this is/was a mistake, and I'm fine not storing this config there anymore.

Currently here is how it seem to work...:

Codemirror instances are created on the create_element methods of codecell.js, and textcell.js (this.code_mirror = new CodeMirror(input_area.get(0), this._options.cm_config); lines.

this._options.cm_config contain likely the lineNumber field which is set on the parent class Cell (from cell.js) generated on the line this._options = utils.mergeopt({}, cell_config_data, class_config_data);, and class_config_data get the config synchronously class_config_data = this.class_config.get_sync();.

Does that make (some) sens ?

@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Sep 23, 2016

Yep, it makes perfect sense. The culprit was https://github.com/jupyter/notebook/blob/master/notebook/static/notebook/js/notebook.js#L1185. I ran into a little error but it's almost there...

@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Oct 1, 2016

@Carreau Let's merge this and I will work off of another branch to try to get notebook.line_numbers to use ConfigWithDefaults.

@gnestor gnestor changed the title [WIP] Persist header and toolbar toggle status in nbconfig Persist header and toolbar toggle status in nbconfig Oct 5, 2016
@Carreau Carreau merged commit 558ed9a into jupyter:master Oct 6, 2016
4 checks passed
4 checks passed
codecov/patch Coverage not affected when comparing 179bb24...6c1fe1f
Details
codecov/project 74.20% (+0.00%) compared to 179bb24
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Carreau
Copy link
Member

@Carreau Carreau commented Oct 6, 2016

You are right, let's try ! Thanks !

@gnestor gnestor deleted the gnestor:persist-header-toggle branch Oct 6, 2016
@gnestor gnestor moved this from Open PRs to Merged PRs in 5.0 Feb 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
5.0
Merged PRs
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.