Respect 'editable' cell metadata #1482

yuvipanda · 2016-05-24T06:54:10Z

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

- 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

yuvipanda · 2016-05-24T06:57:08Z

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:

Add tests
Figure out if setting codemirror option on every focus has performance penalties, or if there is a better location to put that in.

I haven't really written any client side JS in a few years, so be gentle!

takluyver · 2016-05-24T10:08:04Z

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.

takluyver · 2016-05-24T10:09:06Z

notebook/static/notebook/js/codecell.js

@@ -171,6 +171,8 @@ define([
            if (that.keyboard_manager) {
                that.keyboard_manager.enable();
            }
+
+            that.code_mirror.setOption('readOnly', that.is_readonly());


This is on code cells - do we want non-editable Markdown & raw cells as well?

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's MarkdownCell and RawCell, both in textcell.js, but TextCell is the parent class, so if setting it there works, that's good.

Yup, just did :)

takluyver · 2016-05-24T10:10:01Z

Will it be awkward that the two protection options work opposite ways round - deletable: false but readonly: true? Do we want editable: false instead?

yuvipanda · 2016-05-24T13:11:41Z

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

Carreau · 2016-06-04T00:17:01Z

I'm fine with changing the default, be warned that @jhamrick nbgrader might need to be updated as, IIRC, it is the main usecase.

yuvipanda · 2016-06-04T00:54:55Z

Yeah, I think this just needs tests. I'll try to pull in some time next
week to finish this up.

On Fri, Jun 3, 2016 at 5:17 PM, Matthias Bussonnier <
notifications@github.com> wrote:

I'm fine with changing the default, be warned that @jhamrick
https://github.com/jhamrick nbgrader might need to be updated as, IIRC,
it is the main usecase.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1482 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAB23qN_GylSQWIh-WGm5rjLOJwi2ICLks5qIMQBgaJpZM4IlKSo
.

Yuvi Panda T
http://yuvi.in/blog

jhamrick · 2016-06-04T11:03:28Z

I think not editable should imply not deletable as well.

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?

Also thanks @yuvipanda for implementing this!!

minrk · 2016-06-06T11:02:08Z

Yup, any change like that should include automatic upgrading of the old format on load.

gnestor · 2016-08-18T03:38:03Z

@minrk I can work on this. To make sure I understand: if the deletable property is not available for a cell in a notebook, then we should provide a value for it depending on the cell's editable property (e.g. if metadata.editable = false then metadata.deletable = false, otherwise metadata.deletable = true).

minrk · 2016-08-23T08:37:13Z

@gnestor I believe that's correct, yes.

gnestor · 2016-08-24T17:14:22Z

notebook/static/notebook/js/cell.js

     * @method is_deletable
     **/
    Cell.prototype.is_deletable = function () {
-        if (this.metadata.deletable === false) {
+        if (this.metadata.deletable === false || !this.is_editable()) {


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.

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.

gnestor · 2016-09-12T14:59:38Z

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

yuvipanda · 2016-09-12T16:08:30Z

Done! Sorry I haven't had time to finish this up.

gnestor · 2016-09-12T19:06:54Z

@yuvipanda Sorry but I accidentally deleted your branch and I don't think I can restore it on your behalf. I think a git reflog should do the trick. If you can't figure it out either, then I can create a new PR...

yuvipanda · 2016-09-12T19:17:15Z

I've restored it :)

gnestor · 2016-09-14T00:17:46Z

@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 editable and deletable properties. Can you merge pleeez?

Upgrade cell's `editable` and `deletable` metadata if not defined

yuvipanda · 2016-09-14T00:55:09Z

Merged and re-enabled!

gnestor · 2016-09-14T02:06:11Z

@Carreau Can you test this? I have tested and verified that cell metadata will include editable and deletable properties.

To test:

Create a new notebook or open an existing notebook
Inspect notebook in a text editor, there should be no editable or deletable properties for cells
Save and Checkpoint
Inspect notebook in a text editor, there should be editable or deletable properties for cells

Carreau · 2016-09-21T17:11:45Z

LGTM ! Thanks all !

jhamrick · 2017-01-13T21:48:17Z

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.

Carreau · 2017-01-13T22:33:58Z

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

Carreau · 2017-01-13T22:34:55Z

See #1482.

Carreau · 2017-01-13T22:35:29Z

I mean #2039

jhamrick · 2017-01-13T22:43:52Z

Thanks! 😄

Carreau · 2017-01-13T23:05:28Z

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

gnestor · 2017-01-31T00:45:01Z

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

minrk · 2017-01-31T12:59:33Z

I think it's fine. I relabeled it as 4.3.2, since it's already been backported.

Respect 'readonly' cell metadata

87f6b34

- 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

takluyver reviewed May 24, 2016
View reviewed changes

yuvipanda added 2 commits May 24, 2016 08:17

Change is_readonly to is_editable

70afd0c

While I prefer things that default to false than true, in this case it is better to be consistent with is_deletable

Implement 'editable' for textcell type too

66fa05e

yuvipanda changed the title ~~Respect 'readonly' cell metadata~~ Respect 'editable' cell metadata May 24, 2016

Carreau added this to the 5.0 milestone Jul 29, 2016

gnestor self-assigned this Aug 18, 2016

gnestor reviewed Aug 24, 2016
View reviewed changes

gnestor closed this Sep 12, 2016

gnestor deleted the readonly-cells branch September 12, 2016 18:43

yuvipanda restored the readonly-cells branch September 12, 2016 19:17

Upgrade cell's editable and deletable metadata if not defined

767942c

yuvipanda reopened this Sep 14, 2016

Merge pull request #1 from gnestor/patch-1

2237c8e

Upgrade cell's `editable` and `deletable` metadata if not defined

Carreau merged commit dd4b052 into jupyter:master Sep 21, 2016

gnestor added the enhancement label Sep 21, 2016

minrk mentioned this pull request Nov 1, 2016

don't store default editable/deletable metadata #1865

Merged

jhamrick mentioned this pull request Nov 28, 2016

Feature parity with current notebook jupyter/jupyter-js-notebook#87

Open

19 tasks

blink1073 mentioned this pull request Nov 28, 2016

Handle read-only notebook cells jupyterlab/jupyterlab#1312

Closed

Carreau modified the milestones: 4.4, 5.0 Jan 13, 2017

lumberbot-app bot mentioned this pull request Jan 13, 2017

Backport PR #1482 on branch 4.x #2039

Merged

Carreau added a commit that referenced this pull request Jan 14, 2017

Merge pull request #2039 from jupyter/auto-backport-of-pr-1482

0273b3f

Backport PR #1482 on branch 4.x

minrk modified the milestones: 4.3.2, 4.4 Jan 31, 2017

afshin mentioned this pull request Sep 28, 2018

Support enabled field of cell metadata toggling a cell's read-only state. jupyterlab/jupyterlab#5401

Merged

github-actions bot added the status:resolved-locked label Apr 9, 2021

github-actions bot locked as resolved and limited conversation to collaborators Apr 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect 'editable' cell metadata #1482

Respect 'editable' cell metadata #1482

yuvipanda commented May 24, 2016 •

edited

Loading

yuvipanda commented May 24, 2016

takluyver commented May 24, 2016

takluyver May 24, 2016

yuvipanda May 24, 2016

takluyver May 24, 2016

yuvipanda May 24, 2016

takluyver commented May 24, 2016

yuvipanda commented May 24, 2016

Carreau commented Jun 4, 2016

yuvipanda commented Jun 4, 2016

jhamrick commented Jun 4, 2016

minrk commented Jun 6, 2016

gnestor commented Aug 18, 2016

minrk commented Aug 23, 2016

gnestor Aug 24, 2016

jhamrick Aug 24, 2016

gnestor commented Sep 12, 2016

yuvipanda commented Sep 12, 2016

gnestor commented Sep 12, 2016

yuvipanda commented Sep 12, 2016

gnestor commented Sep 14, 2016

yuvipanda commented Sep 14, 2016

gnestor commented Sep 14, 2016 •

edited

Loading

Carreau commented Sep 21, 2016

jhamrick commented Jan 13, 2017

Carreau commented Jan 13, 2017

Carreau commented Jan 13, 2017

Carreau commented Jan 13, 2017

jhamrick commented Jan 13, 2017

Carreau commented Jan 13, 2017

gnestor commented Jan 31, 2017

minrk commented Jan 31, 2017 •

edited

Loading

Respect 'editable' cell metadata #1482

Respect 'editable' cell metadata #1482

Conversation

yuvipanda commented May 24, 2016 • edited Loading

yuvipanda commented May 24, 2016

takluyver commented May 24, 2016

takluyver May 24, 2016

Choose a reason for hiding this comment

yuvipanda May 24, 2016

Choose a reason for hiding this comment

takluyver May 24, 2016

Choose a reason for hiding this comment

yuvipanda May 24, 2016

Choose a reason for hiding this comment

takluyver commented May 24, 2016

yuvipanda commented May 24, 2016

Carreau commented Jun 4, 2016

yuvipanda commented Jun 4, 2016

jhamrick commented Jun 4, 2016

minrk commented Jun 6, 2016

gnestor commented Aug 18, 2016

minrk commented Aug 23, 2016

gnestor Aug 24, 2016

Choose a reason for hiding this comment

jhamrick Aug 24, 2016

Choose a reason for hiding this comment

gnestor commented Sep 12, 2016

yuvipanda commented Sep 12, 2016

gnestor commented Sep 12, 2016

yuvipanda commented Sep 12, 2016

gnestor commented Sep 14, 2016

yuvipanda commented Sep 14, 2016

gnestor commented Sep 14, 2016 • edited Loading

Carreau commented Sep 21, 2016

jhamrick commented Jan 13, 2017

Carreau commented Jan 13, 2017

Carreau commented Jan 13, 2017

Carreau commented Jan 13, 2017

jhamrick commented Jan 13, 2017

Carreau commented Jan 13, 2017

gnestor commented Jan 31, 2017

minrk commented Jan 31, 2017 • edited Loading

yuvipanda commented May 24, 2016 •

edited

Loading

gnestor commented Sep 14, 2016 •

edited

Loading

minrk commented Jan 31, 2017 •

edited

Loading