[pve-devel] [RFC pve-manager 1/2] multi disk edit: add copy button

[Max Carrara]

On 12/6/23 10:25, Dominik Csapak wrote:
> high level, i think i'd find it better to put the button next to the
> remove button (as you suggested in the cover letter)
> 
> my rationale is that the place where the add button is, is a 'neutral' place without connection to an existing disk, but the copy is an
> action related to a specific disk so it makes sense to add it there

I agree! I'll put it there in the v2. I just want to make sure that
I can disable the button as well, since we temporarily disable the
"add" button while constructing the item.

> 
> alternatively you could maybe put it in the disk panel itself, so there
> would still be only a single copy button visible (when selecting the disk) but that might
> 1. look weird (would have to test)
> 2. be hard to place (not sure if there would be space at all)
> 3. make the code uglier (you'd have to call something in the parent class)
> 
> the idea is nice, otherwise see some comments inline
> 
> On 12/5/23 16:44, Max Carrara wrote:
>> Add a copy button that copies the configuration of one disk to a new
>> one.
>>
>> Signed-off-by: Max Carrara <m.carrara at proxmox.com>
>> ---
>>   www/manager6/panel/MultiDiskEdit.js | 78 +++++++++++++++++++++++++----
>>   1 file changed, 68 insertions(+), 10 deletions(-)
>>
>> diff --git a/www/manager6/panel/MultiDiskEdit.js b/www/manager6/panel/MultiDiskEdit.js
>> index ea1f974d..dffbb4c5 100644
>> --- a/www/manager6/panel/MultiDiskEdit.js
>> +++ b/www/manager6/panel/MultiDiskEdit.js
>> @@ -13,14 +13,37 @@ Ext.define('PVE.panel.MultiDiskPanel', {
>>       controller: {
>>       xclass: 'Ext.app.ViewController',
>>   +    disableableButtons: ['addButton', 'copyButton'],
>> +
>>       vmconfig: {},
>>         onAdd: function() {
>>           let me = this;
>> -        me.lookup('addButton').setDisabled(true);
>> +        me.disableButtons();
>>           me.addDisk();
>> -        let count = me.lookup('grid').getStore().getCount() + 1; // +1 is from ide2
>> -        me.lookup('addButton').setDisabled(count >= me.maxCount);
>> +        me.enableButtons();
>> +    },
>> +
>> +    onCopy: function(button, event) {
>> +        let me = this;
>> +        me.disableButtons();
>> +        me.addDisk(true);
>> +        me.enableButtons();
>> +    },
>> +
>> +    disableButtons: function() {
>> +        let me = this;
>> +        for (let buttonName of me.disableableButtons) {
>> +        me.lookup(buttonName).setDisabled(true);
>> +        }
>> +    },
>> +
>> +    enableButtons: function() {
>> +        let me = this;
>> +        let count = me.lookup('grid').getStore().getCount() + 1;
> 
> you lost the ide2 comment here ;)

Woops, my bad!

> 
> 
>> +        for (let buttonName of me.disableableButtons) {
>> +        me.lookup(buttonName).setDisabled(count >= me.maxCount);
>> +        }
>>       },
>>         getNextFreeDisk: function(vmconfig) {
>> @@ -34,7 +57,7 @@ Ext.define('PVE.panel.MultiDiskPanel', {
>>       // define in subclass
>>       diskSorter: undefined,
>>   -    addDisk: function() {
>> +    addDisk: function(copy = false) {
>>           let me = this;
>>           let grid = me.lookup('grid');
>>           let store = grid.getStore();
>> @@ -54,6 +77,21 @@ Ext.define('PVE.panel.MultiDiskPanel', {
>>           })[0];
>>             let panel = me.addPanel(itemId, vmconfig, nextFreeDisk);
>> +
>> +        if (copy) {
>> +        let selection = grid.getSelection()[0];
>> +        let selectedItemId = selection.get('itemId');
>> +        let panelFrom = me.getView().getComponent(selectedItemId);
>> +
>> +        if (!panelFrom) {
>> +            throw "unexpected missing panel";
>> +        }
>> +
>> +        let values = panelFrom.getValues(false, true);
>> +        values.deviceid = nextFreeDisk.id;
> 
> what happens here when the nextfree controller type is not the same?
> the options of the panel differ between them (e.g. ide is not
> capable of setting read only or iothread)

`setValues()` uses `Ext.query()` under the hood - `query()` will return
an empty array if it cannot find anything, so if a field cannot be found,
it will just be silently ignored.

> 
> maybe we should disable that button if there is no free id for
> the same controller type? we do want to copy the config, but
> if we can't then we can't ;)

This approach seems reasonable, though one thing I have noticed when
testing this, is that if you end up running out of IDs, it will simply
add another device with ID 0 and then (rightfully) complain that the
device is already in use. It's not possible to proceed in this case;
either the disk needs to be removed or the bus type needs to be changed.

imo that's acceptable - it does what the user wants but it's up to them
what to do if the IDs run out.

In that case we also don't need to handle changing to a different controller

The add button behaves a little differently though and will automatically
switch to a new bus, so maybe the copy button could to the same, but
I'm not quite sure yet, to be honest.

> 
>> +        panel.setValues(values);
>> +        }
>> +
>>           panel.updateVMConfig(vmconfig);
>>             // we need to setup a validitychange handler, so that we can show
>> @@ -170,7 +208,7 @@ Ext.define('PVE.panel.MultiDiskPanel', {
>>             store.remove(record);
>>           me.getView().remove(record.get('itemId'));
>> -        me.lookup('addButton').setDisabled(false);
>> +        me.enableButtons();
>>           me.updateVMConfig();
>>           me.checkValidity();
>>       },
>> @@ -251,11 +289,31 @@ Ext.define('PVE.panel.MultiDiskPanel', {
>>               ],
>>           },
>>           {
>> -            xtype: 'button',
>> -            reference: 'addButton',
>> -            text: gettext('Add'),
>> -            iconCls: 'fa fa-plus-circle',
>> -            handler: 'onAdd',
>> +            layout: 'hbox',
>> +            border: false,
>> +            defaults: {
>> +            border: false,
>> +            layout: 'anchor',
>> +            flex: 1,
>> +            },
>> +            items: [
>> +            {
>> +                xtype: 'button',
>> +                reference: 'addButton',
>> +                text: gettext('Add'),
>> +                iconCls: 'fa fa-plus-circle',
>> +                handler: 'onAdd',
>> +                margin: '0 2.5 0 0',
>> +            },
>> +            {
>> +                xtype: 'button',
>> +                reference: 'copyButton',
>> +                text: gettext('Copy'),
>> +                iconCls: 'fa fa-files-o',
>> +                handler: 'onCopy',
>> +                margin: '0 0 0 2.5',
>> +            },
>> +            ],
>>           },
>>           {
>>               // dummy field to control wizard validation