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.

Sign up for GitHub

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
Merged

Persist header and toolbar toggle status in nbconfig #1769

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
11d56a2
Define `header` and `toolbar` properties for notebook
gnestor Sep 16, 2016
61eec92
Check header and toolbar toggle status after nbconfig loads
gnestor Sep 20, 2016
c656fc0
Access config via `ConfigWithDefaults` vs. `ConfigSection`
gnestor Sep 20, 2016
6c1fe1f
Move toggle logic from property setters in to actions
gnestor Sep 21, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 6 additions & 21 deletions notebook/static/notebook/js/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,38 +462,23 @@ define(function(require){
env.notebook.show_command_palette();
}
},
'show-all-line-numbers': {
help : 'show line numbers in all cells, and persist the setting',
handler: function(env) {
env.notebook.line_numbers = true;
}
},
'hide-all-line-numbers': {
help : 'hide line numbers in all cells, and persist the setting',
handler: function(env) {
env.notebook.line_numbers = false;
}
},
'toggle-all-line-numbers': {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

help : 'toggles line numbers in all cells, and persist the setting',
icon: 'fa-list-ol',
handler: function(env) {
env.notebook.line_numbers = !env.notebook.line_numbers;
}
},
'toggle-toolbar':{
help: 'hide/show the toolbar',
'toggle-header':{
help: 'hide/show the header',
handler : function(env){
$('div#maintoolbar').toggle();
events.trigger('resize-header.Page');
env.notebook.header = !env.notebook.header;
}
},
'toggle-header':{
help: 'hide/show the header',
'toggle-toolbar':{
help: 'hide/show the toolbar',
handler : function(env){
$('#header-container').toggle();
$('.header-bar').toggle();
events.trigger('resize-header.Page');
env.notebook.toolbar = !env.notebook.toolbar;
}
},
'close-pager': {
Expand Down
85 changes: 67 additions & 18 deletions notebook/static/notebook/js/notebook.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,29 +164,79 @@ import {ShortcutEditor} from 'notebook/js/shortcuteditor';
var that = this;

Object.defineProperty(this, 'line_numbers', {
get: function(){
var d = that.config.data||{}
var cmc = (d['Cell']||{})['cm_config']||{}
return cmc['lineNumbers'] || false;
},
set: function(value){
this.get_cells().map(function(c) {
c.code_mirror.setOption('lineNumbers', value);
})
that.config.update({'Cell':{'cm_config':{'lineNumbers':value}}})
}

})
get: function() {
Copy link
Contributor Author

@gnestor gnestor Sep 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

var d = that.config.data || {};
var cmc = (d['Cell'] || {}) ['cm_config'] || {};
return cmc['lineNumbers'] || false;
},
set: function(value) {
this.get_cells().map(function(c) {
c.code_mirror.setOption('lineNumbers', value);
});
that.config.update({
'Cell': {
'cm_config': {
'lineNumbers':value
}
}
});
}
});

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},
set: function(value) {
if (value === true) {
$('#header-container').show();
$('.header-bar').show();
} else if (value === false) {
$('#header-container').hide();
$('.header-bar').hide();
}
that.events.trigger('resize-header.Page');
that.class_config.set('Header', value);
}
});

Object.defineProperty(this, 'toolbar', {
get: function() {
return that.class_config.get_sync('Toolbar');
},
set: function(value) {
if (value === true) {
$('div#maintoolbar').show();
} else if (value === false) {
$('div#maintoolbar').hide();
}
that.events.trigger('resize-header.Page');
that.class_config.set('Toolbar', value);
}
});

this.class_config.get('Header').then(function(header) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (header === false) {
that.header = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
});

this.class_config.get('Toolbar').then(function(toolbar) {
if (toolbar === false) {
that.toolbar = false;
}
});

// prevent assign to miss-typed properties.
Object.seal(this);
};



Notebook.options_default = {
// can be any cell type, or the special values of
// 'above', 'below', or 'selected' to get the value from another cell.
default_cell_type: 'code'
default_cell_type: 'code',
Header: true,
Toolbar: true
};

/**
Expand Down Expand Up @@ -578,7 +628,7 @@ import {ShortcutEditor} from 'notebook/js/shortcuteditor';
*/
Notebook.prototype.toggle_all_line_numbers = function () {
this.line_numbers = !this.line_numbers;
}
};

/**
* Get the cell above a given cell.
Expand Down Expand Up @@ -3185,4 +3235,3 @@ import {ShortcutEditor} from 'notebook/js/shortcuteditor';
this.events.trigger('checkpoint_deleted.Notebook');
this.load_notebook(this.notebook_path);
};