From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <t.lamprecht@proxmox.com>
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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; Mon, 28 Sep 2020 14:27:36 +0200 (CEST)
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
Stefan Reiter <s.reiter@proxmox.com>
References: <20200924141142.15842-1-s.reiter@proxmox.com>
<20200924141142.15842-2-s.reiter@proxmox.com>
From: Thomas Lamprecht <t.lamprecht@proxmox.com>
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 <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=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 <s.reiter@proxmox.com>
> ---
> 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