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

[Emmanuel Kasper]

On 09/27/2017 10:47 AM, Thomas Lamprecht wrote:
> 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?

but this would mean that a newly added CD ROM drive is not anymore added
to the index 2 on the IDE bus by default ?

I am not sure of the implications of these for the machines type we
emulate, so I would prefer not to change this in this patch serie