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

This enable to keep all attachments in memory and only garbage collect
on the JSON that is saved to disk.
This fixes #1185

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() {