add a dirty class to cell when modified #4362

Closed
wants to merge 2 commits into
from

7 participants

@Carreau
IPython member

Allow to do stuff like :
capture decran 2013-10-08 a 17 21 28

with this css

<style>
.dirty div.input div.prompt{
    color:grey;
}

.dirty div.prompt{
    color:grey;
}
</style>

All cells that have not been executed in the current page have grey prompt, prompt get colored if you execute the cell. It revert back to grey if you ever trigger a 'dirty' on code mirror.

@mgaitan

This would close #2118, right?

@Carreau
IPython member

Not totally as if you 'reload' the page, some cells are greyd, but should are still technically be valid as they reflect the kernel state...

@Carreau
IPython member

Also it would be nice that Modification then Ctrl+Z on codemirror restore the color if relevant.

@damianavila

I like it, but the PR do not include the css... will you add it later? or you have another idea?

@Carreau
IPython member

Not sure if people want the css.
I woudl prefer not to change the behavior for now. I mainly opened the PR to discuss about that.

@mgaitan mgaitan and 1 other commented on an outdated diff Oct 8, 2013
IPython/html/static/notebook/js/cell.js
if (this.code_mirror) {
this.code_mirror.on("change", function(cm, change) {
$([IPython.events]).trigger("set_dirty.Notebook", {value: true});
+ $([IPython.events]).trigger("set_dirty.Cell", {value: true, cell: this});
+ if (that.element !== null){
+ $(that.element).addClass('dirty');
@mgaitan
mgaitan added a line comment Oct 8, 2013

Shouldn't addClass('dirty'); be a consequence of the triggered "set_dirty.Cell" event? So, if a trigger that in another place, the css is automatically set.

@minrk
IPython member
minrk added a line comment Oct 8, 2013

What other place could trigger this? This code fires any time the content of the cell is changed.

@mgaitan
mgaitan added a line comment Oct 8, 2013

the undo/redo event, for example

@minrk
IPython member
minrk added a line comment Oct 8, 2013

undo/redo trigger code_mirror.change, so they will set dirty via this code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Carreau
IPython member

I'll still try to investigate if there is a way to diminish the number of dirty event, like not triggering if the cell is already dirty.

@Carreau
IPython member

could probably use isClean from CM also...

@Carreau
IPython member

I have one other question, which is to know when/wether I should consider that cells other than code cell are dirty ? can they be now or in the future ?

@ellisonbg ellisonbg commented on an outdated diff Oct 9, 2013
IPython/html/static/notebook/js/codecell.js
@@ -265,6 +265,8 @@ var IPython = (function (IPython) {
this.set_input_prompt(content.execution_count);
this.element.removeClass("running");
$([IPython.events]).trigger('set_dirty.Notebook', {value: true});
+ $([IPython.events]).trigger('set_clean.Cell', {value: true, cell: this});
@ellisonbg
IPython member
ellisonbg added a line comment Oct 9, 2013

Isn't passing value: true here redundant?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ellisonbg ellisonbg commented on an outdated diff Oct 9, 2013
IPython/html/static/notebook/js/cell.js
if (this.code_mirror) {
this.code_mirror.on("change", function(cm, change) {
$([IPython.events]).trigger("set_dirty.Notebook", {value: true});
+ $([IPython.events]).trigger("set_dirty.Cell", {value: true, cell: this});
@ellisonbg
IPython member
ellisonbg added a line comment Oct 9, 2013

Isn't passing value: true here redundant?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ellisonbg
IPython member

I am 50/50 on adding this. I know it is simple, but it is not clear how important this functionality really is. In the notebook's lifetime I haven't heard anyone ask for this other than on issue #2118.

@damianavila

@Carreau do you have plans to keep this one live... I really like the idea of this PR ;-)

@damianavila

@Carreau do you have plans to keep this one live... I really like the idea of this PR ;-)

@Carreau pinging you again 😉

@Carreau
IPython member

I'll put that on dev meeting.

@damianavila

I'll put that on dev meeting.

Thanks.

@Carreau
IPython member

I've push a brand new branch on this, which now will mark things clean when using undo/redo and include the CSS.

@jaredly

I definitely want the css (fwiw), but I could stand having it be a config setting. My biggest thing is when I start a new ipython session and open an old notebook, I would like everything marked "not executed" until I run it.

@minrk minrk commented on an outdated diff Jan 13, 2014
IPython/html/static/notebook/js/cell.js
@@ -85,8 +86,20 @@ var IPython = (function (IPython) {
return $.extend(true, {}, _class.options_default, options, overwrite)
}
-
-
+
+ /**
+ * Mark the current cell as clean.
+ *
+ * Store the current CodeMirror edit generation and add
+ * necessary class to the dom to mark the current cell source content
+ * as in sync with it's content.
+ *
+ * @method mark_clean
+ */
+ Cell.prototype.mark_clean = function(){
+ this._clean_generation = this.code_mirror.changeGeneration()
@minrk
IPython member
minrk added a line comment Jan 13, 2014

semicolon;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@minrk minrk commented on an outdated diff Jan 13, 2014
IPython/html/static/notebook/less/notebook.less
@@ -63,3 +63,7 @@ p {
.end_space {
height: 200px;
}
+
+.dirty .prompt{
+ color:gray
@minrk
IPython member
minrk added a line comment Jan 13, 2014

also semicolon;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@damianavila damianavila commented on an outdated diff Jan 14, 2014
IPython/html/static/notebook/js/cell.js
if (this.code_mirror) {
this.code_mirror.on("change", function(cm, change) {
$([IPython.events]).trigger("set_dirty.Notebook", {value: true});
+ that.mark_maybe_dirty()
@damianavila
damianavila added a line comment Jan 14, 2014

semicolon ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ellisonbg
IPython member

About the UI of this:

  • I like the idea of greying the prompt once a cell has been edited and the input/output is out of sync.
  • I think we should be extremely conservative on adding visual styling to the notebook that indicates state change. But this styling is quite subtle and might be OK.
  • I don't like how a reloaded or newly opened notebook has all grey prompts. While I understand that the notion of a cell being dirty that you are using suggests this behavior, I think it is confusing for a user and doesn't reflect the reality that all cells could have been previously run in that kernel and all outputs could be in sync with the inputs. Thus I am -1 on defaulting to the dirty state with grey prompts. All prompts should start out colored and only go grey when a user types into a CM CodeCell.
  • This confusion about how to handle the state when a notebook loaded suggests that we are exposing a leaky abstraction to the user that will cause confusion in many cases.

If we can get colored prompts always upon a notebook loading I am probably +0.5 on the styling. If we have to live with grey prompts upon load I am -1.

Carreau added some commits Dec 25, 2013
@Carreau Carreau add a dirty class to cell element when not in sync
When content of codemirror is not anymore in sync with the output it
created, cell element shoudl now have a "dirty" class applied to it
that make the prompt number gray.
ea1ea0f
@Carreau Carreau fix semicolon, colored on load. 42e5666
@Carreau
IPython member

Updated as per @ellisonbg comment, fixed semicolon and rebased;

Technically you can still edit a cell and have the grey prompt even if in the end your edit has a diff of 0. like adding foo + 3 times backspace.

@ellisonbg
IPython member
@damianavila

All prompts should start out colored and only go grey when a user types into a CM CodeCell.
This confusion about how to handle the state when a notebook loaded suggests that we are exposing a leaky abstraction to the user that will cause confusion in many cases.

Trying to be an user without the details of the implementation, I encounter myself with this problem...
The coloring at the notebook load seems contradictory... you first notice the grey input when you modified a cell... and if it is not execute it, it keeps the gray prompt... you close the notebook and open it again (or reload it) and you will see the coloring instead of the grey prompt... but you expect to stay grey... I know we can explain the behaviour and thell to the user that they have to wait for coloring... but I think that intuitively, this will lead to many confusions...

@damianavila

An it is bummer because I like the idea of this PR... 😡

@Carreau
IPython member
@ellisonbg
IPython member
@damianavila

I think there are some confusing aspects of the original behavior or the
current one. I think that having grey prompts upon a load/refresh is super
confusing as the output is very likely fully in sync with the input and the
kernel.

Maybe we are looking to communicate the wrong thing... I mean, in the current implementation (but we grey prompt at load), we can tell to the user the status of the cells at the notebook frontend level and not at the kernel level... if we try to go deeper (to the kernel level), the complexity grows and we can not use the differential coloring to inform the state... With the coloring, we just can inform if the cell was executed at the notebook UI level in the current "session" and nothing more... I think that this info is enough useful to push forward the PR, but maybe others think different... 😉

@takluyver
IPython member
@Carreau
IPython member

Could we store the dirty state in cell metadata, and restore the colour
when the notebook is loaded? Of course, we can't guarantee that the output
is up to date if someone has changed the notebook file outside IPython, but
in normal cases it would work.

I would like to avoid going to this, but one could store a (a hash of) the last executed input and compare it
to the input also.

But I just feel like we are all trying to show different things with this which are incompatible.

  • input is the one that created this output.
  • input has been runned in this browser session.
  • input has been modified and not re-runned

... and everything liked to kernel.

@ellisonbg
IPython member
@ellisonbg ellisonbg commented on the diff Jan 27, 2014
IPython/html/static/notebook/less/notebook.less
@@ -65,3 +65,7 @@ p {
.end_space {
height: 200px;
}
+
+.dirty .prompt{
+ color:gray;
@ellisonbg
IPython member
ellisonbg added a line comment Jan 27, 2014

I would probably use one of the greys that we already have on the page: maybe #cfcfcf (input area) or #ababab. Also, put this in a less variable in either case so we track colors in one place.

Overall, I like this behavior better. Would like to get @fperez take on this. Adding to this weeks dev meeting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ellisonbg
IPython member

The decision at the dev meeting is that we need to keep thinking about this. There is still some hesitation about the leakiness of this idea, but we want to keep playing with it. Definitely for 3.0.

@Carreau
IPython member

Closing and opening issues #6030 instead.

@Carreau Carreau closed this Jun 22, 2014
@Carreau Carreau deleted the Carreau:dirtycell branch Dec 15, 2014
@minrk minrk modified the milestone: 3.0, no action Feb 14, 2015
@Carreau Carreau referenced this pull request in jupyter/enhancement-proposals Oct 20, 2015
Open

A new notebook document format for improved workflow integration #4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment