Thanks a lot for your careful review, hongliang!

On 2014年04月23日 16:46, Hongliang Wang wrote:
User level comments:
1. Primary disk is allowed to be detached
I think we should prevent user to detach the primary disk. There should not a "Detach" Button after the primary disk.
This is a great idea, but I went through libvirt api, I can't figure out a way to distinguish system disk or the boot disk, anyone who has good idea is welcome.
2. Two minor issues in UI
2.1. Be consistent to provider default selection for select menus

We have default values for device type and device bus, but not storage pool or storage volume. Suggest to automatically select the first option for them.
ACK



2.2. Keep select menu text aligned
ACK

Seems there are two base lines for select menus.



Code level comments are inline below.

On 04/22/2014 05:05 PM, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>

Extend current cdrom disk add to cdrom disk attachment.
Also add 'bus' for user to choose from.

Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com>
---
 ui/css/theme-default/guest-storage-add.css |  16 +++++
 ui/js/src/kimchi.guest_storage_add.main.js | 110 +++++++++++++++++++++++++++--
 ui/pages/guest-storage-add.html.tmpl       |  50 ++++++++++++-
 3 files changed, 168 insertions(+), 8 deletions(-)

diff --git a/ui/css/theme-default/guest-storage-add.css b/ui/css/theme-default/guest-storage-add.css
index 4da8389..e0be9c0 100644
--- a/ui/css/theme-default/guest-storage-add.css
+++ b/ui/css/theme-default/guest-storage-add.css
@@ -52,6 +52,22 @@
     cursor: not-allowed;
 }

+.guest-storage-add-wrapper-label, .guest-storage-add-wrapper-controls {
+    display: inline-block;
+}
+
+.guest-storage-add-wrapper-label {
+    height: 38px;
+    line-height: 38px;
+    margin-top: 5px;
+    vertical-align: top;
+    width: 80px;
+}
+
+.guest-storage-add-wrapper-controls {
+    width: 470px;
+}
+
 #vm-storage-button-add[disabled] {
     background: #c0c0c0;
     color: #ddd;
diff --git a/ui/js/src/kimchi.guest_storage_add.main.js b/ui/js/src/kimchi.guest_storage_add.main.js
index 9e232d7..1f04e55 100644
--- a/ui/js/src/kimchi.guest_storage_add.main.js
+++ b/ui/js/src/kimchi.guest_storage_add.main.js
@@ -18,25 +18,123 @@
 kimchi.guest_storage_add_main = function() {
     var types = [{
         label: 'cdrom',
-        value: 'cdrom'
+        value: 'cdrom',
+        bus: ['ide']
+    },
+    {
+        label: 'disk',
+        value: 'disk',
+        bus: ['virtio', 'ide']
     }];
     kimchi.select('guest-storage-type-list', types);
+    kimchi.select('guest-storage-bus-list', [{label: 'ide', value: 'ide'}]);

     var storageAddForm = $('#form-guest-storage-add');
     var submitButton = $('#guest-storage-button-add');
     var nameTextbox = $('input[name="dev"]', storageAddForm);
     var typeTextbox = $('input[name="type"]', storageAddForm);
+    var busTextbox = $('input[name="bus"]', storageAddForm);
     var pathTextbox = $('input[name="path"]', storageAddForm);
+    var poolTextbox = $('input[name="storagepool"]', storageAddForm);
+    var volTextbox = $('input[name="storagevol"]', storageAddForm);
+
+    typeTextbox.change(function() {
+        $('#guest-storage-bus').selectMenu();
+        var pathObject = {'cdrom': ".path-section", 'disk': '.volume-section'}
+        var selectType = $(this).val();
+        $.each(pathObject, function(type, value) {
+            if(selectType == type){
+                $(value).removeClass('hidden');
+            } else {
+                $(value).addClass('hidden');
+            }
+        });
+
+        $.each(types, function(index, elem){
+            if (selectType == elem.value) {
+                var buses = new Array();
+                $.each(elem.bus, function (index, elem) {
+                    buses[index] = {label: elem, value: elem};
+                });
+                $('#guest-storage-bus').selectMenu("setData", buses);

+                $('#guest-storage-bus-label').text(buses[0].value);
+                $('#guest-storage-bus-type').val(buses[0].value);
Suggest keep consistent on variable naming. e.g.
Text box ID:   #guest-storage-bus-type-textbox
Label ID:        #guest-storage-bus-type-label
ACK
+            }
+        });
+    });
+
+    kimchi.listStoragePools(function(result) {
+        var options = [];
+        if (result && result.length) {
+            $.each(result, function(index, storagePool) {

+                if ((storagePool.state=="active") && (storagePool.type !== 'kimchi-iso')) {
+                    options.push({
+                        label: storagePool.name,
+                        value: storagePool.name
+                        });
+                    }
Seems indent is wrong here. Online code formatting tools can be used.
ACK
+                });
+                kimchi.select('guest-add-storage-pool-list', options);
+        }
+    });
+
+    poolTextbox.change(function() {
+        var options = [];
+        kimchi.listStorageVolumes($(this).val(), function(result) {
+            $('#guest-disk').selectMenu();
+            if (result.length) {
+                $.each(result, function(index, value) {
+                    // Only unused volume can be attached
+                    if (value.ref_cnt == 0) {
+                        options.push({
+                            label: value.name,
+                            value: value.path
+                          });
+                    }
+                });
+            }
+            $('#guest-disk').selectMenu("setData", options);
+        });
+    });
+
+    volTextbox.change(function () {
+        pathTextbox.val(volTextbox.val());
+        pathTextbox.change();
+    });
+
+    typeTextbox.change(function() {
+        var pathObject = {'cdrom': ".path-section", 'disk': '.volume-section'}
+        var selectType = $(this).val();
+        $.each(pathObject, function(type, value) {
+            if(selectType == type){
+                $(value).removeClass('hidden');
+            } else {
+                $(value).addClass('hidden');
+            }
+        });
+
+        $.each(types, function(index, elem){
+            if (selectType == elem.value) {
+                var buses = new Array();
+                $.each(elem.bus, function (index, elem) {
+                    buses[index] = {label: elem, value: elem};
+                });
+                $('#guest-storage-bus').selectMenu("setData", buses);
+                $('#guest-storage-bus-label').text(buses[0].value);
+            }
+        });
+    });

     var submitForm = function(event) {
-        if(submitButton.prop('disabled')) {
+        if (submitButton.prop('disabled')) {
             return false;
         }

         var dev = nameTextbox.val();
         var type = typeTextbox.val();
         var path = pathTextbox.val();
-        if(!path || path === '') {
+        var bus = busTextbox.val();
+        if (!path || path === '') {
             return false;
         }

@@ -49,13 +147,13 @@ kimchi.guest_storage_add_main = function() {
         var settings = {
             vm: kimchi.selectedGuest,
             type: type,
-            path: path
+            path: path,
+            bus: bus
         };

         if(dev && dev !== '') {
             settings['dev'] = dev;
         }
-
         kimchi.addVMStorage(settings, function(result) {
             kimchi.window.close();
             kimchi.topic('kimchi/vmCDROMAttached').publish({
@@ -77,7 +175,7 @@ kimchi.guest_storage_add_main = function() {

     storageAddForm.on('submit', submitForm);
     submitButton.on('click', submitForm);
-    pathTextbox.on('input propertychange', function(event) {
+    pathTextbox.on('change input propertychange', function(event) {
         $(submitButton).prop('disabled', $(this).val() === '');
     });
 };
diff --git a/ui/pages/guest-storage-add.html.tmpl b/ui/pages/guest-storage-add.html.tmpl
index 71e0610..3f92c83 100644
--- a/ui/pages/guest-storage-add.html.tmpl
+++ b/ui/pages/guest-storage-add.html.tmpl
@@ -41,7 +41,7 @@
                 <h2>2. $_("Device Type")</h2>
                 <div class="field">
                     <p class="text-help">
-                        $_("The device type. Currently, only \"cdrom\" is supported.")
+                        $_("The device type. Currently,  \"cdrom\" and \"disk\" are supported.")
                     </p>
                     <div class="btn dropdown popable">
                         <input id="guest-storage-type" name="type" value="cdrom" type="hidden" />
@@ -54,7 +54,51 @@
                 </div>
             </section>
             <section class="form-section">
-                <h2>3. $_("File Path")</h2>
+                <h2>3. $_("Device Bus")</h2>
+                <div class="field">
+                    <div class="btn dropdown popable" id="guest-storage-bus">
+                        <input id="guest-storage-bus-type" name="bus" value='ide' type="hidden" />
+                        <span class="text" id="guest-storage-bus-label">ide</span>
+                        <span class="arrow"></span>
+                        <div class="popover">
+                            <ul class="select-list" id="guest-storage-bus-list" data-target="guest-storage-bus-type" data-label="guest-storage-bus-label"></ul>
+                        </div>
+                    </div>
+                </div>
+            </section>
+                <div class="volume-section hidden">
+                    <section class="form-section">
+                        <h2>4. $_("Storage Pool")</h2>
+                        <div class="field storage-field">
+                            <p class="text-help">
+                                $_("Storage pool which volume located in")</p>
+                                <div class="btn dropdown popable">
+                                    <input value="/storagepools/vg" id="guest-disk-pool" name="storagepool" type="hidden">
+                                        <span class="text" id="guest-disk-pool-label"></span><span class="arrow"></span>
+                                            <div class="popover" style="width: 100%">
+                                                <ul class="select-list" id="guest-add-storage-pool-list" data-target="guest-disk-pool" data-label="guest-disk-pool-label"></ul>
+                                             </div>
+                                </div>
+                        </div>
+                    </section>
+                    <section class="form-section">
+                        <h2>5. $_("Storage Volume")</h2>
+                        <div class="field storage-field">
+                            <p class="text-help">
+                                $_("Storage volume to be attached")</p>
+                                <div class="btn dropdown popable" id="guest-disk">
+                                    <input id="guest-disk-vol" name="storagevol" type="hidden">
+                                        <span class="text" id="guest-disk-vol-label"></span><span class="arrow"></span>
+                                            <div class="popover" style="width: 100%">
+                                                <ul class="select-list" id="guest-add-storage-pool-list" data-target="guest-disk-vol" data-label="guest-disk-vol-label"></ul>
+                                             </div>
+                                </div>
+                        </div>
+                    </section>
+                </div>
+                <div class="path-section">
+                <section class="form-section">
+                <h2>4. $_("File Path")</h2>
                 <div class="field">
                     <p class="text-help">
                         $_("The ISO file path in the server for CDROM.")
@@ -62,6 +106,8 @@
                     <input type="text" class="text" name="path" />
                 </div>
             </section>
+            </div>
+                </fieldset>
         </form>
     </div>
     <footer>