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.carr...@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
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
