From f982c0df3a3d4e3181068c016c1946c8ee6c05bb Mon Sep 17 00:00:00 2001 From: Min RK Date: Wed, 8 Feb 2017 15:20:43 +0100 Subject: [PATCH 1/2] attachment mimebundle values are strings, not lists When first added, the first item of the list contained the attachment. After roundtripping to a file, the list was correctly converted to a single string, the first element of which is a single character, not the full data. --- notebook/static/notebook/js/textcell.js | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/notebook/static/notebook/js/textcell.js b/notebook/static/notebook/js/textcell.js index f3eba5c14f..d1f84a10aa 100644 --- a/notebook/static/notebook/js/textcell.js +++ b/notebook/static/notebook/js/textcell.js @@ -125,7 +125,7 @@ define([ * Add a new attachment to this cell */ this.attachments[key] = {}; - this.attachments[key][mime_type] = [b64_data]; + this.attachments[key][mime_type] = b64_data; }; TextCell.prototype.select = function () { @@ -208,7 +208,7 @@ define([ // to this state, instead of a blank cell this.code_mirror.clearHistory(); // TODO: This HTML needs to be treated as potentially dangerous - // user input and should be handled before set_rendered. + // user input and should be handled before set_rendered. this.set_rendered(data.rendered || ''); this.rendered = false; this.render(); @@ -235,11 +235,11 @@ define([ // We deepcopy the attachments so copied cells don't share the same // objects if (Object.keys(this.attachments).length > 0) { + data.attachments = {}; if (gc_attachments) { // Garbage collect unused attachments : The general idea is to // render the text, and find used attachments like when we // substitute them in render() - data.attachments = {}; var that = this; // To find attachments, rendering to HTML is easier than // searching in the markdown source for the multiple ways you @@ -252,7 +252,7 @@ define([ html.find('img[src^="attachment:"]').each(function (i, h) { h = $(h); var key = h.attr('src').replace(/^attachment:/, ''); - if (key in that.attachments) { + if (that.attachments.hasOwnProperty(key)) { data.attachments[key] = JSON.parse(JSON.stringify( that.attachments[key])); } @@ -263,6 +263,10 @@ define([ }); }); } + if (data.attachments.length === 0) { + // omit attachments dict if no attachments + delete data.attachments; + } } return data; }; @@ -343,7 +347,7 @@ define([ */ var that = this; var pos = this.code_mirror.getCursor(); - var reader = new FileReader; + var reader = new FileReader(); // We can get either a named file (drag'n'drop) or a blob (copy/paste) // We generate names for blobs var key; @@ -412,10 +416,10 @@ define([ h = $(h); var key = h.attr('src').replace(/^attachment:/, ''); - if (key in that.attachments) { + if (that.attachments.hasOwnProperty(key)) { var att = that.attachments[key]; var mime = Object.keys(att)[0]; - h.attr('src', 'data:' + mime + ';base64,' + att[mime][0]); + h.attr('src', 'data:' + mime + ';base64,' + att[mime]); } else { h.attr('src', ''); } From f63880700d2d4f79be1c54c6011e348bb98af615 Mon Sep 17 00:00:00 2001 From: Min RK Date: Wed, 8 Feb 2017 15:35:50 +0100 Subject: [PATCH 2/2] copying cells copies attachments add test to verify --- notebook/static/notebook/js/textcell.js | 12 +++--- notebook/tests/notebook/attachments.js | 55 +++++++++++++++++-------- 2 files changed, 45 insertions(+), 22 deletions(-) diff --git a/notebook/static/notebook/js/textcell.js b/notebook/static/notebook/js/textcell.js index d1f84a10aa..85425cfc85 100644 --- a/notebook/static/notebook/js/textcell.js +++ b/notebook/static/notebook/js/textcell.js @@ -235,12 +235,12 @@ define([ // We deepcopy the attachments so copied cells don't share the same // objects if (Object.keys(this.attachments).length > 0) { - data.attachments = {}; if (gc_attachments) { // Garbage collect unused attachments : The general idea is to // render the text, and find used attachments like when we // substitute them in render() var that = this; + data.attachments = {}; // To find attachments, rendering to HTML is easier than // searching in the markdown source for the multiple ways you // can reference an image in markdown (using []() or a @@ -262,10 +262,12 @@ define([ h.attr('src', ''); }); }); - } - if (data.attachments.length === 0) { - // omit attachments dict if no attachments - delete data.attachments; + if (data.attachments.length === 0) { + // omit attachments dict if no attachments + delete data.attachments; + } + } else { + data.attachments = JSON.parse(JSON.stringify(this.attachments)); } } return data; diff --git a/notebook/tests/notebook/attachments.js b/notebook/tests/notebook/attachments.js index a3be1ee5bc..155e892160 100644 --- a/notebook/tests/notebook/attachments.js +++ b/notebook/tests/notebook/attachments.js @@ -13,8 +13,8 @@ casper.notebook_test(function () { var selector = '#insert_image > a'; this.waitForSelector(selector); this.thenEvaluate(function(sel) { - IPython.notebook.to_markdown(); - var cell = IPython.notebook.get_selected_cell(); + Jupyter.notebook.to_markdown(); + var cell = Jupyter.notebook.get_selected_cell(); cell.set_text(""); cell.unrender(); @@ -43,14 +43,14 @@ casper.notebook_test(function () { // Validate and render the markdown cell this.thenClick('#btn_ok'); this.thenEvaluate(function() { - IPython.notebook.get_cell(0).render(); + Jupyter.notebook.get_cell(0).render(); }); this.wait(300); // Check that an tag has been inserted and that it contains the // image this.then(function() { var img = this.evaluate(function() { - var cell = IPython.notebook.get_cell(0); + var cell = Jupyter.notebook.get_cell(0); var img = $("div.text_cell_render").find("img"); return { src: img.attr("src"), @@ -78,7 +78,7 @@ casper.notebook_test(function () { // append a new cell this.append_cell('', 'markdown'); this.thenEvaluate(function() { - IPython.notebook.select_next(); + Jupyter.notebook.select_next(); }); // and paste the attachments into it @@ -89,39 +89,60 @@ casper.notebook_test(function () { // check that the new cell has attachments this.then(function() { var cell_attachments = this.evaluate(function() { - return IPython.notebook.get_selected_cell().attachments; + return Jupyter.notebook.get_selected_cell().attachments; }); var orig_cell_attachments = this.evaluate(function() { - return IPython.notebook.get_cell(0).attachments; + return Jupyter.notebook.get_cell(0).attachments; }); - var clip = this.evaluate(function() { return IPython.notebook.clipboard_attachments; }); // Check that the two cells have the same attachments this.test.assertEquals(cell_attachments, orig_cell_attachments, - "both cells have the attachments"); + "pasted attachments ok"); + }); + + // copy/paste cell includes attachments + selector = '#copy_cell > a'; + this.waitForSelector(selector); + this.thenClick(selector); + + selector = '#paste_cell_below > a'; + this.waitForSelector(selector); + this.thenClick(selector); + + // check that the new cell has attachments + this.then(function() { + var cell_attachments = this.evaluate(function() { + return Jupyter.notebook.get_selected_cell().attachments; + }); + var orig_cell_attachments = this.evaluate(function() { + return Jupyter.notebook.get_cell(0).attachments; + }); + // Check that the two cells have the same attachments + this.test.assertEquals(cell_attachments, orig_cell_attachments, + "pasted cell has attachments"); }); var nbname = 'attachments_test.ipynb'; this.thenEvaluate(function(nbname) { - IPython.notebook.set_notebook_name(nbname); + Jupyter.notebook.set_notebook_name(nbname); }, {nbname:nbname}); // -- Save the notebook. This should cause garbage collection for the // second cell (since we just pasted the attachments but there is no // markdown referencing them) this.thenEvaluate(function(nbname) { - IPython._checkpoint_created = false; + Jupyter._checkpoint_created = false; require(['base/js/events'], function (events) { events.on('checkpoint_created.Notebook', function (evt, data) { - IPython._checkpoint_created = true; + Jupyter._checkpoint_created = true; }); }); - IPython.notebook.save_checkpoint(); + Jupyter.notebook.save_checkpoint(); }, {nbname:nbname}); this.waitFor(function () { return this.evaluate(function(){ - return IPython._checkpoint_created; + return Jupyter._checkpoint_created; }); }); @@ -151,16 +172,16 @@ casper.notebook_test(function () { this.waitFor(this.kernel_running); this.waitFor(function() { return this.evaluate(function () { - return IPython && IPython.notebook && true; + return Jupyter && Jupyter.notebook && true; }); }); this.then(function() { var cell0 = this.evaluate(function() { - return IPython.notebook.get_cell(0); + return Jupyter.notebook.get_cell(0); }); var cell1 = this.evaluate(function() { - return IPython.notebook.get_cell(1); + return Jupyter.notebook.get_cell(1); }); this.test.assert('black_square_22.png' in cell0.attachments, 'cell0 has kept its attachments');