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 1D8A3B8B11 for ; Wed, 6 Dec 2023 10:25:37 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id F288436CF4 for ; Wed, 6 Dec 2023 10:25:36 +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 10:25:36 +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 26BA8424DF for ; Wed, 6 Dec 2023 10:25:36 +0100 (CET) Message-ID: <8b887fa6-fe4e-4f88-9677-12037d140ec8@proxmox.com> Date: Wed, 6 Dec 2023 10:25:35 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Proxmox VE development discussion , Max Carrara References: <20231205154458.268660-1-m.carrara@proxmox.com> <20231205154458.268660-3-m.carrara@proxmox.com> From: Dominik Csapak In-Reply-To: <20231205154458.268660-3-m.carrara@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.018 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 09:25:37 -0000 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 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 ;) > + 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) 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 ;) > + 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