From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 06C5AB8AE8 for ; Wed, 6 Dec 2023 11:50:26 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D8B9C37A46 for ; Wed, 6 Dec 2023 11:49:55 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 6 Dec 2023 11:49:55 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id ECC9D40CE9 for ; Wed, 6 Dec 2023 11:49:54 +0100 (CET) Message-ID: <822650fb-1dd2-4342-bb53-e8906ca0db60@proxmox.com> Date: Wed, 6 Dec 2023 11:49:53 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Dominik Csapak , Proxmox VE development discussion References: <20231205154458.268660-1-m.carrara@proxmox.com> <20231205154458.268660-3-m.carrara@proxmox.com> <8b887fa6-fe4e-4f88-9677-12037d140ec8@proxmox.com> From: Max Carrara In-Reply-To: <8b887fa6-fe4e-4f88-9677-12037d140ec8@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.060 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pve-devel] [RFC pve-manager 1/2] multi disk edit: add copy button X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 06 Dec 2023 10:50:26 -0000 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 >> --- >>   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