-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Respect 'editable' cell metadata #1482
Conversation
- Readonly cells can not be split or merged - Readonly cells can be executed - End users can still modify this by editing the metadata themsevles directly
Should readonly cells be deletable? I'm unsure! Perhaps it's ok to keep those two orthogonal, but might be confusing? Should it provide some other visual indicator of being readonly? Probably, although I'm not sure what form that should take. TODO:
I haven't really written any client side JS in a few years, so be gentle! |
From a UI point of view, I'd expect readonly cells to also be undeletable; it's hard to imagine a scenario where you want a non-editable but deletable cell. |
@@ -171,6 +171,8 @@ define([ | |||
if (that.keyboard_manager) { | |||
that.keyboard_manager.enable(); | |||
} | |||
|
|||
that.code_mirror.setOption('readOnly', that.is_readonly()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is on code cells - do we want non-editable Markdown & raw cells as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I've duplicated it in textcell.js, which seems to be the only other cell type defined? I can't really set this in the parent cell.js since no codemirror stuff happens there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's MarkdownCell and RawCell, both in textcell.js
, but TextCell is the parent class, so if setting it there works, that's good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, just did :)
Will it be awkward that the two protection options work opposite ways round - |
In general I prefer defaults to false than defaults to true, but since it is already 'deletable: false' rather than 'undeletable: true', I guess it makes sense to keep it consistent. |
While I prefer things that default to false than true, in this case it is better to be consistent with is_deletable
I'm fine with changing the default, be warned that @jhamrick nbgrader might need to be updated as, IIRC, it is the main usecase. |
Yeah, I think this just needs tests. I'll try to pull in some time next On Fri, Jun 3, 2016 at 5:17 PM, Matthias Bussonnier <
Yuvi Panda T |
I think not editable should imply not deletable as well. I'm fine with changing the default but if we do that (go from Also thanks @yuvipanda for implementing this!! |
Yup, any change like that should include automatic upgrading of the old format on load. |
@minrk I can work on this. To make sure I understand: if the |
@gnestor I believe that's correct, yes. |
* @method is_deletable | ||
**/ | ||
Cell.prototype.is_deletable = function () { | ||
if (this.metadata.deletable === false) { | ||
if (this.metadata.deletable === false || !this.is_editable()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with changing the default but if we do that (go from deletable to undeletable) could we build in something that would automatically update the notebook metadata if the old deletable metadata is found?
@jhamrick This block will handle cases where metadata.deletable
is undefined (is_deletable()
will return true
). Do you think that we need to write that value back to the metadata? It seems like this would only be necessary if a notebook were to be used in another client that doesn't define an is_deletable()
equivalent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think it should update the old metadata. As @minrk said:
Yup, any change like that should include automatic upgrading of the old format on load.
So even before it gets to the point of checking if the cell is deleteable, ideally the old metadata format should be updated.
@yuvipanda Can you enable "Allow edits from maintainers" for this PR? It's a new Github feature and should be available in the right side bar for this PR. |
Done! Sorry I haven't had time to finish this up. |
@yuvipanda Sorry but I accidentally deleted your branch and I don't think I can restore it on your behalf. I think a |
I've restored it :) |
@yuvipanda I think the "Allow edits from maintainers" permissions were reset after I deleted your branch. I submitted a PR to your fork that will upgrade a notebook's metadata on load to include |
Upgrade cell's `editable` and `deletable` metadata if not defined
Merged and re-enabled! |
@Carreau Can you test this? I have tested and verified that cell metadata will include To test:
|
LGTM ! Thanks all ! |
Would it be possible to include this for the next release (4.4) rather than 5.0? This is a really important feature for nbgrader to have that I thought would have ended up in the 4.3 release, but apparently it didn't. It would be unfortunate if all the instructors waiting on this feature have to wait until 5.0 for it. |
let's see how hard it is to backport. @meeseeksdev backport to 4.x |
- Readonly cells can not be split or merged or deleted - Readonly cells can be executed - End users can still modify this by editing the metadata themsevles directly Fixes 1402
See #1482. |
I mean #2039 |
Thanks! 😄 |
No problem. Also I whitelisted you so you can mention the bot to do backport or other stuff. |
Backport PR #1482 on branch 4.x
Can we release this in 4.3.2? It's currently in 4.x branch and we're about to do a 4.3.2 release... |
I think it's fine. I relabeled it as 4.3.2, since it's already been backported. |
themsevles directly
Fixes #1402