[pve-devel] [PATCH manager v3 1/8] Do not use 'autoselect' as a boolean when preselecting a bus

[Thomas Lamprecht]

On 09/26/2017 02:17 PM, Emmanuel Kasper wrote:
> The bus selector is displayed when we add a Hard Disk or CD Drive.
> When it is displayed, we *always* preselect the next available
> slot on the controller of our choice.
> So this test is not needed.
> 
> We keep the test on the string value of 'autoselect' to select
> a bus position when adding a CD Drive.

Looks good, at least with git show -w :)
Reviewed-by: Thomas Lamprecht <t.lamprecht at proxmox.com>

A side comment still inline below.

> ---
>   www/manager6/form/ControllerSelector.js | 53 ++++++++++++++++-----------------
>   www/manager6/qemu/HDEdit.js             |  2 +-
>   2 files changed, 27 insertions(+), 28 deletions(-)
> 
> diff --git a/www/manager6/form/ControllerSelector.js b/www/manager6/form/ControllerSelector.js
> index 002134ef..045e7d0d 100644
> --- a/www/manager6/form/ControllerSelector.js
> +++ b/www/manager6/form/ControllerSelector.js
> @@ -58,37 +58,36 @@ Ext.define('PVE.form.ControllerSelector', {
>   	var me = this;
>   
>   	me.vmconfig = Ext.apply({}, vmconfig);
> -	if (autoSelect) {
> -	    var clist = ['ide', 'virtio', 'scsi', 'sata'];
> -	    if (autoSelect === 'cdrom') {
> -		clist = ['ide', 'scsi', 'sata'];
> -		if (!Ext.isDefined(me.vmconfig.ide2)) {
> -		    me.down('field[name=controller]').setValue('ide');
> -		    me.down('field[name=deviceid]').setValue(2);
> -		    return;
> -		}
> -	    } else  {
> -		// in most cases we want to add a disk to the same controller
> -		// we previously used
> -		clist = me.sortByPreviousUsage(me.vmconfig, clist);
> +
> +	var clist = ['ide', 'virtio', 'scsi', 'sata'];
> +	if (autoSelect === 'cdrom') {
> +	    clist = ['ide', 'scsi', 'sata'];
> +	    if (!Ext.isDefined(me.vmconfig.ide2)) {

AFAIS, this is just the fastpath?
As performance is rarely a problem when iterating a very small array,
maybe just remove this all but the line where clist gets re-set
and let the general-purpose code below handle this case too?

> +		me.down('field[name=controller]').setValue('ide');
> +		me.down('field[name=deviceid]').setValue(2);
> +		return;
>   	    }
> +	} else  {
> +	    // in most cases we want to add a disk to the same controller
> +	    // we previously used
> +	    clist = me.sortByPreviousUsage(me.vmconfig, clist);

Could be pulled out of the else?
But as said, this patch seems still OK to me, the hunks I'm
commenting here show only up as a effect of the indent change,
thus I saw this.

> +	}
>   
> -	    Ext.Array.each(clist, function(controller) {
> -		var confid, i;
> -		if ((controller === 'virtio' && me.noVirtIO) ||
> +	Ext.Array.each(clist, function(controller) {
> +	    var confid, i;
> +	    if ((controller === 'virtio' && me.noVirtIO) ||
>   		    (controller === 'scsi' && me.noScsi)) {
> -		    return; //continue
> -		}
> -		me.down('field[name=controller]').setValue(controller);
> -		for (i = 0; i <= PVE.form.ControllerSelector.maxIds[controller]; i++) {
> -		    confid = controller + i.toString();
> -		    if (!Ext.isDefined(me.vmconfig[confid])) {
> -			me.down('field[name=deviceid]').setValue(i);
> -			return false; // break
> -		    }
> +		return; //continue
> +	    }
> +	    me.down('field[name=controller]').setValue(controller);
> +	    for (i = 0; i <= PVE.form.ControllerSelector.maxIds[controller]; i++) {
> +		confid = controller + i.toString();
> +		if (!Ext.isDefined(me.vmconfig[confid])) {
> +		    me.down('field[name=deviceid]').setValue(i);
> +		    return false; // break
>   		}
> -	    });
> -	}
> +	    }
> +	});
>   	me.down('field[name=deviceid]').validate();
>       },
>   
> diff --git a/www/manager6/qemu/HDEdit.js b/www/manager6/qemu/HDEdit.js
> index e7535f50..8a415d8b 100644
> --- a/www/manager6/qemu/HDEdit.js
> +++ b/www/manager6/qemu/HDEdit.js
> @@ -140,7 +140,7 @@ Ext.define('PVE.qemu.HDInputPanel', {
>   	me.vmconfig = vmconfig;
>   
>   	if (me.bussel) {
> -	    me.bussel.setVMConfig(vmconfig, true);
> +	    me.bussel.setVMConfig(vmconfig);
>   	}
>   	if (me.unusedDisks) {
>   	    var disklist = [];
>