From 5f9e75dcef60cf7c9727e1b44be55806722886b1 Mon Sep 17 00:00:00 2001
From: Min RK <benjaminrk@gmail.com>
Date: Mon, 12 Jan 2015 14:01:33 -0800
Subject: [PATCH] cleanup kernelspec loading

- kernel_selector.set_kernel validates selection and triggers 'spec_changed.Kernel'. It does not start the session anymore.
- notebook calls kernel_selector.set_kernel when:
  - kernelspec is in notebook metadata
  - session is loaded (e.g. no kernelspec metadata)
- notebook starts session, loads metadata on spec_changed.kernel

The only case where starting the session is not triggered by spec_changed is on notebook load with no kernel metadata
---
 .../html/static/notebook/js/kernelselector.js | 39 ++++++++-----------
 IPython/html/static/notebook/js/notebook.js   | 28 ++++++-------
 2 files changed, 28 insertions(+), 39 deletions(-)

diff --git a/IPython/html/static/notebook/js/kernelselector.js b/IPython/html/static/notebook/js/kernelselector.js
index 9be8936f2..7cebca6c0 100644
--- a/IPython/html/static/notebook/js/kernelselector.js
+++ b/IPython/html/static/notebook/js/kernelselector.js
@@ -126,28 +126,27 @@ define([
         }
     };
 
-    KernelSelector.prototype.change_kernel = function (kernel_name) {
-        /**
-         * TODO, have a methods to set kernel spec directly ?
-         **/
+    KernelSelector.prototype.set_kernel = function (kernel_name) {
+        /** set the kernel by name, ensuring kernelspecs have been loaded, first */
+        var that = this;
+        return this.loaded.then(function () {
+            that._set_kernel(kernel_name);
+        });
+    };
+
+    KernelSelector.prototype._set_kernel = function (kernel_name) {
+        /** Actually set the kernel (kernelspecs have been loaded) */
+        console.log("_set_kernel", kernel_name, this.current_selection);
         if (kernel_name === this.current_selection) {
+            // only trigger event if value changed
             return;
         }
         var ks = this.kernelspecs[kernel_name];
-        
-        try {
-            this.notebook.start_session(kernel_name);
-        } catch (e) {
-            if (e.name === 'SessionAlreadyStarting') {
-                console.log("Cannot change kernel while waiting for pending session start.");
-            } else {
-                // unhandled error
-                throw e;
-            }
-            // only trigger spec_changed if change was successful
+        if (this.notebook._session_starting) {
+            console.log("Cannot change kernel while waiting for pending session start.");
             return;
         }
-        console.log('spec', kernel_name, ks);
+        this.current_selection = kernel_name;
         this.events.trigger('spec_changed.Kernel', ks);
     };
 
@@ -189,13 +188,7 @@ define([
         this.events.on('spec_changed.Kernel', $.proxy(this._spec_changed, this));
 
         this.events.on('kernel_created.Session', function (event, data) {
-            if (data.kernel.name !== that.current_selection) {
-                // If we created a 'python' session, we only know if it's Python
-                // 3 or 2 on the server's reply, so we fire the event again to
-                // set things up.
-                var ks = that.kernelspecs[data.kernel.name];
-                that.events.trigger('spec_changed.Kernel', ks);
-            }
+            that.set_kernel(data.kernel.name);
         });
         
         var logo_img = this.element.find("img.current_kernel_logo");
diff --git a/IPython/html/static/notebook/js/notebook.js b/IPython/html/static/notebook/js/notebook.js
index d0e591a71..77a1aa7be 100644
--- a/IPython/html/static/notebook/js/notebook.js
+++ b/IPython/html/static/notebook/js/notebook.js
@@ -248,6 +248,10 @@ define([
         this.events.on('spec_changed.Kernel', function(event, data) {
             that.metadata.kernelspec = 
                 {name: data.name, display_name: data.spec.display_name};
+            // start session if the current session isn't already correct
+            if (!(this.session && this.session.kernel && this.session.kernel.name === data.name)) {
+                that.start_session(data.name);
+            }
         });
 
         this.events.on('kernel_ready.Kernel', function(event, data) {
@@ -1763,19 +1767,6 @@ define([
         this.notebook_path = data.path;
         var trusted = true;
         
-        // Trigger an event changing the kernel spec - this will set the default
-        // codemirror mode
-        if (this.metadata.kernelspec !== undefined) {
-            // TODO shoudl probably not trigger here, 
-            // should call the kernel selector, or custom.{js|css} not loaded.
-            if(this.kernel_selector){
-                // technically not perfect, we should check that the kernelspec matches
-                this.kernel_selector.change_kernel(this.metadata.kernelspec.name);
-            } else {
-                console.log('do not have handle on kernel_selector');
-            }
-        }
-        
         // Set the codemirror mode from language_info metadata
         if (this.metadata.language_info !== undefined) {
             var langinfo = this.metadata.language_info;
@@ -2193,8 +2184,6 @@ define([
             this.nbformat_minor = nbmodel.nbformat_minor;
         }
         
-        // Create the session after the notebook is completely loaded to prevent
-        // code execution upon loading, which is a security risk.
         if (this.session === null) {
             var kernel_name;
             if (this.metadata.kernelspec) {
@@ -2203,7 +2192,14 @@ define([
             } else {
                 kernel_name = utils.get_url_param('kernel_name');
             }
-            this.start_session(kernel_name);
+            if (kernel_name) {
+                // setting kernel_name here triggers start_session
+                this.kernel_selector.set_kernel(kernel_name);
+            } else {
+                // start a new session with the server's default kernel
+                // spec_changed events will fire after kernel is loaded
+                this.start_session();
+            }
         }
         // load our checkpoint list
         this.list_checkpoints();