Merge pull request #1271 from julienr/fix_1185

Move the unused attachments garbage collection logic to TextCell.toJSON.

notebook/static/notebook/js/cell.js

@@ -350,16 +350,6 @@ define([
         }
     };
 
-    /**
-     * Garbage collects unused attachments in this cell
-     * @method remove_unused_attachments
-     */
-    Cell.prototype.remove_unused_attachments = function () {
-        // Cell subclasses which support attachments should override this
-        // and keep them when needed
-        this.attachments = {};
-    };
-
     /**
      * Delegates keyboard shortcut handling to either Jupyter keyboard
      * manager when in command mode, or CodeMirror when in edit mode
@@ -756,11 +746,6 @@ define([
         this.element = cell;
     };
 
-    UnrecognizedCell.prototype.remove_unused_attachments = function () {
-        // Do nothing to avoid removing attachments from a possible future
-        // attachment-supporting cell type
-    };
-
     UnrecognizedCell.prototype.bind_events = function () {
         Cell.prototype.bind_events.apply(this, arguments);
         var cell = this;

notebook/static/notebook/js/notebook.js

@@ -2403,19 +2403,9 @@ define(function (require) {
     };
 
     /**
-     * Garbage collects unused attachments in all the cells
-     */
-    Notebook.prototype.remove_unused_attachments = function() {
-      var cells = this.get_cells();
-      for (var i = 0; i < cells.length; i++) {
-          var cell = cells[i];
-          cell.remove_unused_attachments();
-      }
-    };
-
-    /**
+     Move the unused attachments garbage collection logic to TextCell.toJSON.
      * Load a notebook from JSON (.ipynb).
-     * 
+     *
      * @param {object} data - JSON representation of a notebook
      */
     Notebook.prototype.fromJSON = function (data) {
@@ -2478,7 +2468,7 @@ define(function (require) {
             if (cell.cell_type === 'code' && !cell.output_area.trusted) {
                 trusted = false;
             }
-            cell_array[i] = cell.toJSON();
+            cell_array[i] = cell.toJSON(true);
         }
         var data = {
             cells: cell_array,
@@ -2527,17 +2517,12 @@ define(function (require) {
      * Save this notebook on the server. This becomes a notebook instance's
      * .save_notebook method *after* the entire notebook has been loaded.
      *
-     * manual_save will be true if the save was manually trigered by the user
      */
-    Notebook.prototype.save_notebook = function (check_last_modified,
-                                                 manual_save) {
+    Notebook.prototype.save_notebook = function (check_last_modified) {
         if (check_last_modified === undefined) {
             check_last_modified = true;
         }
-        if (manual_save === undefined) {
-            manual_save = false;
-        }
-        
+
         var error;
         if (!this._fully_loaded) {
             error = new Error("Load failed, save is disabled");
@@ -2553,13 +2538,6 @@ define(function (require) {
         // the notebook as needed.
         this.events.trigger('before_save.Notebook');
 
-        // Garbage collect unused attachments. Only do this for manual save
-        // to avoid removing unused attachments while the user is editing if
-        // an autosave gets triggered in the midle of an edit
-        if (manual_save) {
-            this.remove_unused_attachments();
-        }
-
         // Create a JSON model to be sent to the server.
         var model = {
             type : "notebook",
@@ -2743,7 +2721,7 @@ define(function (require) {
         var parent = utils.url_path_split(this.notebook_path)[0];
         var p;
         if (this.dirty) {
-            p = this.save_notebook(true, true);
+            p = this.save_notebook(true);
         } else {
             p = Promise.resolve();
         }
@@ -3013,7 +2991,7 @@ define(function (require) {
      */
     Notebook.prototype.save_checkpoint = function () {
         this._checkpoint_after_save = true;
-        this.save_notebook(true, true);
+        this.save_notebook(true);
     };
     
     /**

notebook/static/notebook/js/textcell.js

@@ -123,44 +123,6 @@ define([
         this.attachments[key][mime_type] = [b64_data];
     };
 
-    TextCell.prototype.remove_unused_attachments = function () {
-        // The general idea is to render the text, find attachment like when
-        // we substitute them in render() and mark used attachments by adding
-        // a temporary .used property to them.
-        if (Object.keys(this.attachments).length > 0) {
-            var that = this;
-            // To find unused 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 HTML <img> tag)
-            var text = this.get_text();
-            marked(text, function (err, html) {
-                html = security.sanitize_html(html);
-                html = $($.parseHTML(html));
-                html.find('img[src^="attachment:"]').each(function (i, h) {
-                    h = $(h);
-                    var key = h.attr('src').replace(/^attachment:/, '');
-                    if (key in that.attachments) {
-                        that.attachments[key].used = true;
-                    }
-
-                    // This is to avoid having the browser do a GET request
-                    // on the invalid attachment: URL
-                    h.attr('src', '');
-                });
-            });
-
-            for (var key in this.attachments) {
-                if (this.attachments[key].used === undefined) {
-                    console.log('Dropping unused attachment ' + key);
-                    delete this.attachments[key];
-                } else {
-                    // Remove temporary property
-                    delete this.attachments[key].used;
-                }
-            }
-        }
-    }
-
     TextCell.prototype.select = function () {
         var cont = Cell.prototype.select.apply(this, arguments);
         if (cont) {
@@ -230,7 +192,6 @@ define([
      */
     TextCell.prototype.fromJSON = function (data) {
         Cell.prototype.fromJSON.apply(this, arguments);
-        console.log('data cell_type : ' + data.cell_type + ' this.cell_type : ' + this.cell_type);
         if (data.cell_type === this.cell_type) {
             if (data.attachments !== undefined) {
                 this.attachments = data.attachments;
@@ -251,18 +212,52 @@ define([
     };
 
     /** Generate JSON from cell
+     * @param {bool} gc_attachments - If true, will remove unused attachments
+     *               from the returned JSON
      * @return {object} cell data serialised to json
      */
-    TextCell.prototype.toJSON = function () {
+    TextCell.prototype.toJSON = function (gc_attachments) {
+        if (gc_attachments === undefined) {
+            gc_attachments = false;
+        }
+
         var data = Cell.prototype.toJSON.apply(this);
         data.source = this.get_text();
         if (data.source == this.placeholder) {
             data.source = "";
         }
-        // Deepcopy the attachments so copied cells don't share the same
+
+        // We deepcopy the attachments so copied cells don't share the same
         // objects
         if (Object.keys(this.attachments).length > 0) {
-            data.attachments = JSON.parse(JSON.stringify(this.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
+                // can reference an image in markdown (using []() or a
+                // HTML <img>)
+                var text = this.get_text();
+                marked(text, function (err, html) {
+                    html = security.sanitize_html(html);
+                    html = $($.parseHTML(html));
+                    html.find('img[src^="attachment:"]').each(function (i, h) {
+                        h = $(h);
+                        var key = h.attr('src').replace(/^attachment:/, '');
+                        if (key in that.attachments) {
+                            data.attachments[key] = JSON.parse(JSON.stringify(
+                                that.attachments[key]));
+                        }
+
+                        // This is to avoid having the browser do a GET request
+                        // on the invalid attachment: URL
+                        h.attr('src', '');
+                    });
+                });
+            }
         }
         return data;
     };

notebook/tests/notebook/attachments.js

@@ -100,11 +100,59 @@ casper.notebook_test(function () {
                                "both cells have the attachments");
     });
 
+    var nbname = 'attachments_test.ipynb';
+    this.thenEvaluate(function(nbname) {
+        IPython.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() {
+    this.thenEvaluate(function(nbname) {
+        IPython._checkpoint_created = false;
+        require(['base/js/events'], function (events) {
+            events.on('checkpoint_created.Notebook', function (evt, data) {
+                IPython._checkpoint_created = true;
+            });
+        });
+
         IPython.notebook.save_checkpoint();
+    }, {nbname:nbname});
+
+    this.waitFor(function () {
+        return this.evaluate(function(){
+            return IPython._checkpoint_created;
+        });
+    });
+
+    this.then(function(){
+        this.open_dashboard();
+    });
+
+
+    this.then(function(){
+        var notebook_url = this.evaluate(function(nbname){
+            var escaped_name = encodeURIComponent(nbname);
+            var return_this_thing = null;
+            $("a.item_link").map(function (i,a) {
+                if (a.href.indexOf(escaped_name) >= 0) {
+                    return_this_thing = a.href;
+                    return;
+                }
+            });
+            return return_this_thing;
+        }, {nbname:nbname});
+        this.test.assertNotEquals(notebook_url, null, "Escaped URL in notebook list");
+        // open the notebook
+        this.open(notebook_url);
+    });
+
+    // wait for the notebook
+    this.waitFor(this.kernel_running);
+    this.waitFor(function() {
+        return this.evaluate(function () {
+            return IPython && IPython.notebook && true;
+        });
     });
 
     this.then(function() {