Re: [pve-devel] [PATCH manager 2/2] ui: improve boot order editor with 'bootorder' support

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

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.
> 
> 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.
> 
> Support for configs still using the old properties is given, with the
> first write automatically updating the VM config to use the new one.
> 
> 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.
> 
> Note that it is very well possible to disable all boot devices, in which
> 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.
> 

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 "reorder" as
  alias) to rows
* Add a # column which shows the "order", i.e., similar to our firewall rules
* Use another renderer for the Options view, IMO the "A > B > C" makes people
  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 changing 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(-)
> 
> diff --git a/www/manager6/qemu/BootOrderEdit.js b/www/manager6/qemu/BootOrderEdit.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,
>  
> -    bootdisk: undefined,
> -    selection: [],
> -    list: [],
> -    comboboxes: [],
> -
> -    isBootDisk: function(value) {
> +    isDisk: function(value, cdrom) {
>  	return PVE.Utils.bus_match.test(value);
>      },
>  
> -    setVMConfig: function(vmconfig) {
> -	var me = this;
> -	me.vmconfig = vmconfig;
> -	var order = me.vmconfig.boot || 'cdn';
> -	me.bootdisk = me.vmconfig.bootdisk || undefined;
> -
> -	// get the first 3 characters
> -	// ignore the rest (there should never be more than 3)
> -	me.selection = order.split('').slice(0,3);
> -
> -	// build bootdev list
> -	me.list = [];
> -	Ext.Object.each(me.vmconfig, function(key, value) {
> -	    if (me.isBootDisk(key) &&
> -		!(/media=cdrom/).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));
>      },
>  
> -    onGetValues: function(values) {
> -	var me = this;
> -	var order = me.selection.join('');
> -	var res = { boot: order };
> +    setVMConfig: function(vmconfig) {
> +	let me = this;
> +	me.vmconfig = vmconfig;
>  
> -	if  (me.bootdisk && order.indexOf('c') !== -1) {
> -	    res.bootdisk = me.bootdisk;
> +	me.store.removeAll();
> +
> +	let bootorder;
> +	if (me.vmconfig.bootorder) {
> +	    bootorder = (/^\s*$/).test(me.vmconfig.bootorder) ? [] :
> +		me.vmconfig.bootorder
> +		    .split(',')
> +		    .map(dev => ({name: dev, enabled: true}));
>  	} else {
> -	    res['delete'] = 'bootdisk';
> +	    // legacy style, transform to new bootorder
> +	    let order = me.vmconfig.boot || 'cdn';
> +	    let bootdisk = me.vmconfig.bootdisk || undefined;
> +
> +	    // get the first 3 characters
> +	    // ignore the rest (there should never be more than 3)
> +	    let orderList = order.split('').slice(0,3);
> +
> +	    // build bootdev list
> +	    bootorder = [];
> +	    for (let i = 0; i < orderList.length; i++) {
> +		let list = [];
> +		if (orderList[i] === 'c') {
> +		    if (bootdisk !== undefined && me.vmconfig[bootdisk]) {
> +			list.push(bootdisk);
> +		    }
> +		} else if (orderList[i] === 'd') {
> +		    Ext.Object.each(me.vmconfig, function(key, value) {
> +			if (me.isDisk(key) && (/media=cdrom/).test(value)) {
> +			    list.push(key);
> +			}
> +		    });
> +		} else if (orderList[i] === '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 => bootorder.push({name: dev, enabled: true}));
> +	    }
>  	}
>  
> +	// add disabled devices as well
> +	let disabled = [];
> +	Ext.Object.each(me.vmconfig, function(key, value) {
> +	    if (me.isBootdev(key, value) &&
> +		!Ext.Array.some(bootorder, x => x.name === key))
> +	    {
> +		disabled.push(key);
> +	    }
> +	});
> +	disabled.sort();
> +	disabled.forEach(dev => bootorder.push({name: dev, enabled: false}));
> +
> +	bootorder.forEach(entry => {
> +	    entry.desc = me.vmconfig[entry.name];
> +	});
> +
> +	me.store.insert(0, bootorder);
> +	me.store.fireEvent("update");
> +    },
> +
> +    calculateValue: function() {
> +	let me = this;
> +	return me.store.getData().items
> +	    .filter(x => x.data.enabled)
> +	    .map(x => x.data.name)
> +	    .join(',');
> +    },
> +
> +    onGetValues: function() {
> +	let me = this;
> +	// Note: we allow an empty value, so no 'delete' option
> +	let res = { bootorder: me.calculateValue() };
>  	return res;
>      },
>  
> -    recomputeSelection: function(combobox, newVal, oldVal) {
> -	var me = this.up('#inputpanel');
> -	me.selection = [];
> -	me.comboboxes.forEach(function(item) {
> -	    var val = item.getValue();
> -
> -	    // when selecting an already selected item,
> -	    // switch it around
> -	    if ((val === newVal || (me.isBootDisk(val) && me.isBootDisk(newVal))) &&
> -		item.name !== combobox.name &&
> -		newVal !== '__none__') {
> -		// swap items
> -		val = oldVal;
> -	    }
> -
> -	    // push 'c','d' or 'n' in the array
> -	    if (me.isBootDisk(val)) {
> -		me.selection.push('c');
> -		me.bootdisk = val;
> -	    } else if (val === 'd' ||
> -		       val === 'n') {
> -		me.selection.push(val);
> -	    }
> -	});
> -
> -	me.recomputeList();
> -    },
> -
> -    recomputeList: function(){
> -	var me = this;
> -	// set the correct values in the kvcomboboxes
> -	var cnt = 0;
> -	me.comboboxes.forEach(function(item) {
> -	    if (cnt === 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] !== 'c')?me.selection[cnt]:me.bootdisk);
> -	    } else if (cnt === 0){
> -		item.setValue('');
> -	    } else {
> -		item.setValue('__none__');
> -	    }
> -	    cnt++;
> -	    item.resumeEvent('change');
> -	    item.validate();
> -	});
> -    },
> -
>      initComponent : function() {
> -	var me = this;
> +	let me = this;
>  
> -	// this has to be done here, because of
> -	// the way our inputPanel class handles items
> -	me.comboboxes = [
> -		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 = false;
> +	let marker = 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 = Ext.create('Ext.form.field.Display', {
> +	    userCls: 'pmx-hint',
> +	    value: gettext('Warning: No devices selected, the VM will probably not boot!'),
> +	});
> +
> +	me.store = Ext.create('Ext.data.Store', {
> +	    model: 'pve-boot-order-entry',
> +	    listeners: {
> +		update: function() {
> +		    this.commitChanges();
> +		    let val = me.calculateValue();
> +		    if (!marker.originalValue) {
> +			marker.originalValue = val;
> +		    }
> +		    inUpdate = true;
> +		    marker.setValue(val);
> +		    inUpdate = false;
> +		    marker.checkDirty();
> +		    emptyWarning.setHidden(val !== '');
>  		}
> -	    }),
> -		Ext.createWidget('proxmoxKVComboBox', {
> -		fieldLabel: gettext('Boot device') + " 3",
> -		labelWidth: 120,
> -		name: 'bd3',
> -		allowBlank: false,
> -		listeners: {
> -		    change: me.recomputeSelection
> +	    }
> +	});
> +
> +	let grid = 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 = 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', {
>      }],
>  
>      subject: gettext('Boot Order'),
> +    width: 600,
>  
>      initComponent : function() {
> -	var me = this;
> +	let me = this;
>  	me.callParent();
>  	me.load({
>  	    success: function(response, options) {
> diff --git a/www/manager6/qemu/Options.js b/www/manager6/qemu/Options.js
> 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' : undefined,
> -		multiKey: ['boot', 'bootdisk'],
> +		multiKey: ['boot', 'bootdisk', 'bootorder'],
>  		renderer: function(order, metaData, record, rowIndex, colIndex, store, pending) {
> +		    let bootorder = me.getObjectValue('bootorder', undefined, pending);
> +		    if (bootorder) {
> +			let list = bootorder.split(',');
> +			let ret = '';
> +			list.forEach(dev => {
> +			    if (ret !== '') {
> +				ret += ' > ';
> +			    }
> +			    let desc = dev;
> +			    ret += desc;
> +			});
> +			return ret;
> +		    }
> +
> +		    // legacy style and fallback
>  		    var i;
>  		    var text = '';
>  		    var bootdisk = me.getObjectValue('bootdisk', undefined, pending);
> @@ -99,20 +117,20 @@ Ext.define('PVE.qemu.Options', {
>  		    for (i = 0; i < order.length; i++) {
>  			var sel = order.substring(i, i + 1);
>  			if (text) {
> -			    text += ', ';
> +			    text += ' > ';
>  			}
>  			if (sel === 'c') {
>  			    if (bootdisk) {
> -				text += "Disk '" + bootdisk + "'";
> +				text += bootdisk;
>  			    } else {
> -				text += "Disk";
> +				text += "disk";
>  			    }
>  			} else if (sel === 'n') {
> -			    text += 'Network';
> +			    text += 'any net';
>  			} else if (sel === 'a') {
> -			    text += 'Floppy';
> +			    text += 'floppy';
>  			} else if (sel === 'd') {
> -			    text += 'CD-ROM';
> +			    text += 'any cdrom';
>  			} else {
>  			    text += sel;
>  			}
>