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

[Dominik Csapak <d.csapak@proxmox.com>]

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 <m.carrara@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 ;)


> +	    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