From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 62FBA61BFB for ; Mon, 28 Sep 2020 14:28:09 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 4C5D92E027 for ; Mon, 28 Sep 2020 14:27:39 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 7957F2E01B for ; Mon, 28 Sep 2020 14:27:36 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 3C03B43F6D for ; Mon, 28 Sep 2020 14:27:36 +0200 (CEST) To: Proxmox VE development discussion , Stefan Reiter References: <20200924141142.15842-1-s.reiter@proxmox.com> <20200924141142.15842-2-s.reiter@proxmox.com> From: Thomas Lamprecht Message-ID: <42187382-6a0c-4c03-71c9-4e401fd6fe33@proxmox.com> Date: Mon, 28 Sep 2020 14:27:33 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:82.0) Gecko/20100101 Thunderbird/82.0 MIME-Version: 1.0 In-Reply-To: <20200924141142.15842-2-s.reiter@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL -0.165 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.011 Looks like a legit reply (A) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH manager 2/2] ui: improve boot order editor with 'bootorder' support X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 28 Sep 2020 12:28:09 -0000 On 24.09.20 16:11, Stefan Reiter wrote: > The new 'bootorder' property can express many more scenarios than the > old 'boot'/'bootdisk' ones. Update the editor so it can handle it. >=20 > Features a grid with all supported boot devices which can be reordered > using drag-and-drop, as well as toggled on and off with an inline > checkbox. >=20 > Support for configs still using the old properties is given, with the > first write automatically updating the VM config to use the new one. >=20 > The renderer for the Options panel is updated with support for the new > format, and the display format for the fallback is changed to make it > look uniform. >=20 > Note that it is very well possible to disable all boot devices, in whic= h > case an empty 'bootorder: ' will be stored to the config file. I'm not > sure what that would be useful for, but there's no reason to forbid it > either, just warn the user that it's probably not what he wants. >=20 It's not bad, I like it in general! I'd a few things though. * Add a order icon ( https://fontawesome.com/v4.7.0/icon/bars has "reorde= r" as alias) to rows * Add a # column which shows the "order", i.e., similar to our firewall r= ules * Use another renderer for the Options view, IMO the "A > B > C" makes pe= ople think to hard, rather just use number, i.e., to something like "1. scsi0, 2. Any CD-ROM, ..."# Also, please keep the case and style of CD-ROM as is, no point in chang= ing that. > Signed-off-by: Stefan Reiter > --- > www/manager6/qemu/BootOrderEdit.js | 310 +++++++++++++++++------------= > www/manager6/qemu/Options.js | 32 ++- > 2 files changed, 210 insertions(+), 132 deletions(-) >=20 > diff --git a/www/manager6/qemu/BootOrderEdit.js b/www/manager6/qemu/Boo= tOrderEdit.js > index 19d5d50a..b1236bbd 100644 > --- a/www/manager6/qemu/BootOrderEdit.js > +++ b/www/manager6/qemu/BootOrderEdit.js > @@ -1,149 +1,208 @@ > +Ext.define('pve-boot-order-entry', { > + extend: 'Ext.data.Model', > + fields: [ > + {name: 'name', type: 'string'}, > + {name: 'enabled', type: 'bool'}, > + {name: 'desc', type: 'string'}, > + ] > +}); > + > Ext.define('PVE.qemu.BootOrderPanel', { > extend: 'Proxmox.panel.InputPanel', > alias: 'widget.pveQemuBootOrderPanel', > + > vmconfig: {}, // store loaded vm config > + store: undefined, > + originalValue: undefined, > =20 > - bootdisk: undefined, > - selection: [], > - list: [], > - comboboxes: [], > - > - isBootDisk: function(value) { > + isDisk: function(value, cdrom) { > return PVE.Utils.bus_match.test(value); > }, > =20 > - setVMConfig: function(vmconfig) { > - var me =3D this; > - me.vmconfig =3D vmconfig; > - var order =3D me.vmconfig.boot || 'cdn'; > - me.bootdisk =3D me.vmconfig.bootdisk || undefined; > - > - // get the first 3 characters > - // ignore the rest (there should never be more than 3) > - me.selection =3D order.split('').slice(0,3); > - > - // build bootdev list > - me.list =3D []; > - Ext.Object.each(me.vmconfig, function(key, value) { > - if (me.isBootDisk(key) && > - !(/media=3Dcdrom/).test(value)) { > - me.list.push([key, "Disk '" + key + "'"]); > - } > - }); > - > - me.list.push(['d', 'CD-ROM']); > - me.list.push(['n', gettext('Network')]); > - me.list.push(['__none__', Proxmox.Utils.noneText]); > - > - me.recomputeList(); > - > - me.comboboxes.forEach(function(box) { > - box.resetOriginalValue(); > - }); > + isBootdev: function(dev, value) { > + return this.isDisk(dev) || > + (/^net\d+/).test(dev) || > + (/^hostpci\d+/).test(dev) || > + ((/^usb\d+/).test(dev) && !(/spice/).test(value)); > }, > =20 > - onGetValues: function(values) { > - var me =3D this; > - var order =3D me.selection.join(''); > - var res =3D { boot: order }; > + setVMConfig: function(vmconfig) { > + let me =3D this; > + me.vmconfig =3D vmconfig; > =20 > - if (me.bootdisk && order.indexOf('c') !=3D=3D -1) { > - res.bootdisk =3D me.bootdisk; > + me.store.removeAll(); > + > + let bootorder; > + if (me.vmconfig.bootorder) { > + bootorder =3D (/^\s*$/).test(me.vmconfig.bootorder) ? [] : > + me.vmconfig.bootorder > + .split(',') > + .map(dev =3D> ({name: dev, enabled: true})); > } else { > - res['delete'] =3D 'bootdisk'; > + // legacy style, transform to new bootorder > + let order =3D me.vmconfig.boot || 'cdn'; > + let bootdisk =3D me.vmconfig.bootdisk || undefined; > + > + // get the first 3 characters > + // ignore the rest (there should never be more than 3) > + let orderList =3D order.split('').slice(0,3); > + > + // build bootdev list > + bootorder =3D []; > + for (let i =3D 0; i < orderList.length; i++) { > + let list =3D []; > + if (orderList[i] =3D=3D=3D 'c') { > + if (bootdisk !=3D=3D undefined && me.vmconfig[bootdisk]) { > + list.push(bootdisk); > + } > + } else if (orderList[i] =3D=3D=3D 'd') { > + Ext.Object.each(me.vmconfig, function(key, value) { > + if (me.isDisk(key) && (/media=3Dcdrom/).test(value)) { > + list.push(key); > + } > + }); > + } else if (orderList[i] =3D=3D=3D 'n') { > + Ext.Object.each(me.vmconfig, function(key, value) { > + if ((/^net\d+/).test(key)) { > + list.push(key); > + } > + }); > + } > + > + // Object.each iterates in random order, sort alphabetically > + list.sort(); > + list.forEach(dev =3D> bootorder.push({name: dev, enabled: true})); > + } > } > =20 > + // add disabled devices as well > + let disabled =3D []; > + Ext.Object.each(me.vmconfig, function(key, value) { > + if (me.isBootdev(key, value) && > + !Ext.Array.some(bootorder, x =3D> x.name =3D=3D=3D key)) > + { > + disabled.push(key); > + } > + }); > + disabled.sort(); > + disabled.forEach(dev =3D> bootorder.push({name: dev, enabled: false})= ); > + > + bootorder.forEach(entry =3D> { > + entry.desc =3D me.vmconfig[entry.name]; > + }); > + > + me.store.insert(0, bootorder); > + me.store.fireEvent("update"); > + }, > + > + calculateValue: function() { > + let me =3D this; > + return me.store.getData().items > + .filter(x =3D> x.data.enabled) > + .map(x =3D> x.data.name) > + .join(','); > + }, > + > + onGetValues: function() { > + let me =3D this; > + // Note: we allow an empty value, so no 'delete' option > + let res =3D { bootorder: me.calculateValue() }; > return res; > }, > =20 > - recomputeSelection: function(combobox, newVal, oldVal) { > - var me =3D this.up('#inputpanel'); > - me.selection =3D []; > - me.comboboxes.forEach(function(item) { > - var val =3D item.getValue(); > - > - // when selecting an already selected item, > - // switch it around > - if ((val =3D=3D=3D newVal || (me.isBootDisk(val) && me.isBootDisk= (newVal))) && > - item.name !=3D=3D combobox.name && > - newVal !=3D=3D '__none__') { > - // swap items > - val =3D oldVal; > - } > - > - // push 'c','d' or 'n' in the array > - if (me.isBootDisk(val)) { > - me.selection.push('c'); > - me.bootdisk =3D val; > - } else if (val =3D=3D=3D 'd' || > - val =3D=3D=3D 'n') { > - me.selection.push(val); > - } > - }); > - > - me.recomputeList(); > - }, > - > - recomputeList: function(){ > - var me =3D this; > - // set the correct values in the kvcomboboxes > - var cnt =3D 0; > - me.comboboxes.forEach(function(item) { > - if (cnt =3D=3D=3D 0) { > - // never show 'none' on first combobox > - item.store.loadData(me.list.slice(0, me.list.length-1)); > - } else { > - item.store.loadData(me.list); > - } > - item.suspendEvent('change'); > - if (cnt < me.selection.length) { > - item.setValue((me.selection[cnt] !=3D=3D 'c')?me.selection[cnt]:me.b= ootdisk); > - } else if (cnt =3D=3D=3D 0){ > - item.setValue(''); > - } else { > - item.setValue('__none__'); > - } > - cnt++; > - item.resumeEvent('change'); > - item.validate(); > - }); > - }, > - > initComponent : function() { > - var me =3D this; > + let me =3D this; > =20 > - // this has to be done here, because of > - // the way our inputPanel class handles items > - me.comboboxes =3D [ > - Ext.createWidget('proxmoxKVComboBox', { > - fieldLabel: gettext('Boot device') + " 1", > - labelWidth: 120, > - name: 'bd1', > - allowBlank: false, > - listeners: { > - change: me.recomputeSelection > + // for dirty marking and reset detection > + let inUpdate =3D false; > + let marker =3D Ext.create('Ext.form.field.Base', { > + hidden: true, > + itemId: 'marker', > + setValue: function(val) { > + // on form reset, go back to original state > + if (!inUpdate) { > + me.setVMConfig(me.vmconfig); > } > - }), > - Ext.createWidget('proxmoxKVComboBox', { > - fieldLabel: gettext('Boot device') + " 2", > - labelWidth: 120, > - name: 'bd2', > - allowBlank: false, > - listeners: { > - change: me.recomputeSelection > + > + // not a subclass, so no callParent; just do it manually > + this.setRawValue(this.valueToRaw(val)); > + return this.mixins.field.setValue.call(this, val); > + } > + }); > + > + let emptyWarning =3D Ext.create('Ext.form.field.Display', { > + userCls: 'pmx-hint', > + value: gettext('Warning: No devices selected, the VM will probabl= y not boot!'), > + }); > + > + me.store =3D Ext.create('Ext.data.Store', { > + model: 'pve-boot-order-entry', > + listeners: { > + update: function() { > + this.commitChanges(); > + let val =3D me.calculateValue(); > + if (!marker.originalValue) { > + marker.originalValue =3D val; > + } > + inUpdate =3D true; > + marker.setValue(val); > + inUpdate =3D false; > + marker.checkDirty(); > + emptyWarning.setHidden(val !=3D=3D ''); > } > - }), > - Ext.createWidget('proxmoxKVComboBox', { > - fieldLabel: gettext('Boot device') + " 3", > - labelWidth: 120, > - name: 'bd3', > - allowBlank: false, > - listeners: { > - change: me.recomputeSelection > + } > + }); > + > + let grid =3D Ext.create('Ext.grid.Panel', { > + store: me.store, It seems to me that this all could be made more schematic without to much= work, could be nice. > + margin: '0 0 5 0', > + columns: [ > + { > + xtype: 'checkcolumn', > + header: gettext('Enabled'), > + dataIndex: 'enabled', > + width: 70, > + sortable: false, > + hideable: false, > + draggable: false, > + }, > + { > + header: gettext('Device'), > + dataIndex: 'name', > + width: 70, > + sortable: false, > + hideable: false, > + draggable: false, > + }, > + { > + header: gettext('Description'), > + dataIndex: 'desc', > + flex: true, > + sortable: false, > + hideable: false, > + draggable: false, > + }, > + ], > + viewConfig: { > + plugins: { > + ptype: 'gridviewdragdrop', > + dragText:gettext('Drag and drop to reorder'), > } > - }) > - ]; > - Ext.apply(me, { items: me.comboboxes }); > + }, > + listeners: { > + drop: function() { > + // doesn't fire automatically on reorder > + me.store.fireEvent("update"); > + } > + }, > + }); > + > + let info =3D Ext.create('Ext.Component', { > + html: gettext('Drag and drop to reorder'), > + }); > + > + Ext.apply(me, { items: [grid, info, emptyWarning, marker] }); > + > me.callParent(); > } > }); > @@ -157,9 +216,10 @@ Ext.define('PVE.qemu.BootOrderEdit', { > }], > =20 > subject: gettext('Boot Order'), > + width: 600, > =20 > initComponent : function() { > - var me =3D this; > + let me =3D this; > me.callParent(); > me.load({ > success: function(response, options) { > diff --git a/www/manager6/qemu/Options.js b/www/manager6/qemu/Options.j= s > index 20f6ffbb..490d2f54 100644 > --- a/www/manager6/qemu/Options.js > +++ b/www/manager6/qemu/Options.js > @@ -86,12 +86,30 @@ Ext.define('PVE.qemu.Options', { > bootdisk: { > visible: false > }, > + bootorder: { > + visible: false > + }, > boot: { > header: gettext('Boot Order'), > defaultValue: 'cdn', > editor: caps.vms['VM.Config.Disk'] ? 'PVE.qemu.BootOrderEdit' : unde= fined, > - multiKey: ['boot', 'bootdisk'], > + multiKey: ['boot', 'bootdisk', 'bootorder'], > renderer: function(order, metaData, record, rowIndex, colIndex, stor= e, pending) { > + let bootorder =3D me.getObjectValue('bootorder', undefined, pend= ing); > + if (bootorder) { > + let list =3D bootorder.split(','); > + let ret =3D ''; > + list.forEach(dev =3D> { > + if (ret !=3D=3D '') { > + ret +=3D ' > '; > + } > + let desc =3D dev; > + ret +=3D desc; > + }); > + return ret; > + } > + > + // legacy style and fallback > var i; > var text =3D ''; > var bootdisk =3D me.getObjectValue('bootdisk', undefined, pendin= g); > @@ -99,20 +117,20 @@ Ext.define('PVE.qemu.Options', { > for (i =3D 0; i < order.length; i++) { > var sel =3D order.substring(i, i + 1); > if (text) { > - text +=3D ', '; > + text +=3D ' > '; > } > if (sel =3D=3D=3D 'c') { > if (bootdisk) { > - text +=3D "Disk '" + bootdisk + "'"; > + text +=3D bootdisk; > } else { > - text +=3D "Disk"; > + text +=3D "disk"; > } > } else if (sel =3D=3D=3D 'n') { > - text +=3D 'Network'; > + text +=3D 'any net'; > } else if (sel =3D=3D=3D 'a') { > - text +=3D 'Floppy'; > + text +=3D 'floppy'; > } else if (sel =3D=3D=3D 'd') { > - text +=3D 'CD-ROM'; > + text +=3D 'any cdrom'; > } else { > text +=3D sel; > } >=20