public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [RFC proxmox-widget-toolkit, pve-manager 0/2] Copy Button For Wizard Disk Configuration
@ 2023-12-05 15:44 Max Carrara
  2023-12-05 15:44 ` [pve-devel] [RFC proxmox-widget-toolkit 1/2] input panel: add `raw` parameter to function `getValues` Max Carrara
  2023-12-05 15:44 ` [pve-devel] [RFC pve-manager 1/2] multi disk edit: add copy button Max Carrara
  0 siblings, 2 replies; 7+ messages in thread
From: Max Carrara @ 2023-12-05 15:44 UTC (permalink / raw)
  To: pve-devel

When creating a VM for testing purposes, e.g. a VM that's supposed
to have 8 disks to test something in regards to ZFS, it's necessary
to manually edit the configuration and repeatedly click on all desired
options by hand - for every single disk. I always wished there was a
copy button, so I decided to just implement it myself.

These two patches are sent as RFC because I'm not 100% sure about the
some of the naming, approaches and UI design decisions I have made,
so it would be nice to get some feedback on that.

In particular:
- Maybe it's better to name the button "duplicate" instead of "copy"?
  * But that means it doesn't fit in the same row as the "add" button
- Put the copy button next to the little icon for the delete button
  instead, displaying it for each list item?


Max Carrara (1):
  input panel: add `raw` parameter to function `getValues`

 src/panel/InputPanel.js | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

Max Carrara (1):
  multi disk edit: add copy button

 www/manager6/panel/MultiDiskEdit.js | 78 +++++++++++++++++++++++++----
 1 file changed, 68 insertions(+), 10 deletions(-)


-- 
2.39.2





^ permalink raw reply	[flat|nested] 7+ messages in thread

* [pve-devel] [RFC proxmox-widget-toolkit 1/2] input panel: add `raw` parameter to function `getValues`
  2023-12-05 15:44 [pve-devel] [RFC proxmox-widget-toolkit, pve-manager 0/2] Copy Button For Wizard Disk Configuration Max Carrara
@ 2023-12-05 15:44 ` Max Carrara
  2023-12-06  9:13   ` Dominik Csapak
  2023-12-05 15:44 ` [pve-devel] [RFC pve-manager 1/2] multi disk edit: add copy button Max Carrara
  1 sibling, 1 reply; 7+ messages in thread
From: Max Carrara @ 2023-12-05 15:44 UTC (permalink / raw)
  To: pve-devel

This parameter may be used to circumvent calls to `onGetValues`.

Also adds a docstring for the function.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/panel/InputPanel.js | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/src/panel/InputPanel.js b/src/panel/InputPanel.js
index 34150ef..723be42 100644
--- a/src/panel/InputPanel.js
+++ b/src/panel/InputPanel.js
@@ -31,7 +31,16 @@ Ext.define('Proxmox.panel.InputPanel', {
 	return values;
     },
 
-    getValues: function(dirtyOnly) {
+    /**
+     * Returns the submit data from the panel's form fields.
+     *
+     * @param {boolean} dirtyOnly `true` to return only dirty fields
+     * (fields that have been changed from their original value).
+     * @param {boolean} raw `true` to prevent calling
+     * {@link Proxmox.panel.InputPanel#onGetValues onGetValues} and
+     * instead return the original submit data.
+     */
+    getValues: function(dirtyOnly, raw) {
 	let me = this;
 
 	if (Ext.isFunction(me.onGetValues)) {
@@ -46,6 +55,10 @@ Ext.define('Proxmox.panel.InputPanel', {
 	    }
 	});
 
+	if (raw) {
+	    return values;
+	}
+
 	return me.onGetValues(values);
     },
 
-- 
2.39.2





^ permalink raw reply	[flat|nested] 7+ messages in thread

* [pve-devel] [RFC pve-manager 1/2] multi disk edit: add copy button
  2023-12-05 15:44 [pve-devel] [RFC proxmox-widget-toolkit, pve-manager 0/2] Copy Button For Wizard Disk Configuration Max Carrara
  2023-12-05 15:44 ` [pve-devel] [RFC proxmox-widget-toolkit 1/2] input panel: add `raw` parameter to function `getValues` Max Carrara
@ 2023-12-05 15:44 ` Max Carrara
  2023-12-06  9:25   ` Dominik Csapak
  1 sibling, 1 reply; 7+ messages in thread
From: Max Carrara @ 2023-12-05 15:44 UTC (permalink / raw)
  To: pve-devel

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





^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [pve-devel] [RFC proxmox-widget-toolkit 1/2] input panel: add `raw` parameter to function `getValues`
  2023-12-05 15:44 ` [pve-devel] [RFC proxmox-widget-toolkit 1/2] input panel: add `raw` parameter to function `getValues` Max Carrara
@ 2023-12-06  9:13   ` Dominik Csapak
  2023-12-06  9:59     ` Max Carrara
  0 siblings, 1 reply; 7+ messages in thread
From: Dominik Csapak @ 2023-12-06  9:13 UTC (permalink / raw)
  To: Proxmox VE development discussion, Max Carrara

hi, some comment inline

On 12/5/23 16:44, Max Carrara wrote:
> This parameter may be used to circumvent calls to `onGetValues`.
> 
> Also adds a docstring for the function.
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
>   src/panel/InputPanel.js | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/src/panel/InputPanel.js b/src/panel/InputPanel.js
> index 34150ef..723be42 100644
> --- a/src/panel/InputPanel.js
> +++ b/src/panel/InputPanel.js
> @@ -31,7 +31,16 @@ Ext.define('Proxmox.panel.InputPanel', {
>   	return values;
>       },
>   
> -    getValues: function(dirtyOnly) {
> +    /**
> +     * Returns the submit data from the panel's form fields.
> +     *
> +     * @param {boolean} dirtyOnly `true` to return only dirty fields
> +     * (fields that have been changed from their original value).
> +     * @param {boolean} raw `true` to prevent calling
> +     * {@link Proxmox.panel.InputPanel#onGetValues onGetValues} and
> +     * instead return the original submit data.
> +     */

nice to see these things documented, not sure about the format, but i 
guess we could adapt such a standard format (maybe at one point we
could even generate nicer docs for this :) )

another thing is the interface, adding a parameter works and can be ok
but wouldn't it be nicer to invent a new 'getRawValues'?

this would then not change the signature of the original function and
is more in line with what extjs does internally
(getValue/getRawValue/etc.)

not a hard requirement for me though

a slight tangent thought:

after looking, it seems we never use(d?) the dirtyOnly
parameter... so it might be nice to remove that

we couldn't even have used it anywhere because we always
set it to false when 'onGetValues' is  a function
so we'd  need to set it to something different, but
later we call it unconditionally as a function(?!?)

but that is rather unrelated, just confused me
(it seems it was "always" this way, so maybe
there is some hidden svn history that i'm not seeing
where the code would make more sense)

> +    getValues: function(dirtyOnly, raw) {
>   	let me = this;
>   
>   	if (Ext.isFunction(me.onGetValues)) {
> @@ -46,6 +55,10 @@ Ext.define('Proxmox.panel.InputPanel', {
>   	    }
>   	});
>   
> +	if (raw) {
> +	    return values;
> +	}
> +
>   	return me.onGetValues(values);
>       },
>   




^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [pve-devel] [RFC pve-manager 1/2] multi disk edit: add copy button
  2023-12-05 15:44 ` [pve-devel] [RFC pve-manager 1/2] multi disk edit: add copy button Max Carrara
@ 2023-12-06  9:25   ` Dominik Csapak
  2023-12-06 10:49     ` Max Carrara
  0 siblings, 1 reply; 7+ messages in thread
From: Dominik Csapak @ 2023-12-06  9:25 UTC (permalink / raw)
  To: Proxmox VE development discussion, Max Carrara

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




^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [pve-devel] [RFC proxmox-widget-toolkit 1/2] input panel: add `raw` parameter to function `getValues`
  2023-12-06  9:13   ` Dominik Csapak
@ 2023-12-06  9:59     ` Max Carrara
  0 siblings, 0 replies; 7+ messages in thread
From: Max Carrara @ 2023-12-06  9:59 UTC (permalink / raw)
  To: Dominik Csapak, Proxmox VE development discussion

On 12/6/23 10:13, Dominik Csapak wrote:
> hi, some comment inline
> 
> On 12/5/23 16:44, Max Carrara wrote:
>> This parameter may be used to circumvent calls to `onGetValues`.
>>
>> Also adds a docstring for the function.
>>
>> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
>> ---
>>   src/panel/InputPanel.js | 15 ++++++++++++++-
>>   1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/panel/InputPanel.js b/src/panel/InputPanel.js
>> index 34150ef..723be42 100644
>> --- a/src/panel/InputPanel.js
>> +++ b/src/panel/InputPanel.js
>> @@ -31,7 +31,16 @@ Ext.define('Proxmox.panel.InputPanel', {
>>       return values;
>>       },
>>   -    getValues: function(dirtyOnly) {
>> +    /**
>> +     * Returns the submit data from the panel's form fields.
>> +     *
>> +     * @param {boolean} dirtyOnly `true` to return only dirty fields
>> +     * (fields that have been changed from their original value).
>> +     * @param {boolean} raw `true` to prevent calling
>> +     * {@link Proxmox.panel.InputPanel#onGetValues onGetValues} and
>> +     * instead return the original submit data.
>> +     */
> 
> nice to see these things documented, not sure about the format, but i guess we could adapt such a standard format (maybe at one point we
> could even generate nicer docs for this :) )

That's just JSDoc - I haven't really seen it used anywhere here (and to
be honest, also didn't bother to look) so I figured I'd just use it here
and see how people respond. ;) Ext JS uses it too, so.

> 
> another thing is the interface, adding a parameter works and can be ok
> but wouldn't it be nicer to invent a new 'getRawValues'?
> 
> this would then not change the signature of the original function and
> is more in line with what extjs does internally
> (getValue/getRawValue/etc.)
> 
> not a hard requirement for me though

Absolutely not opposed to this - I think then the code is more exlicit
anyway. E.g. `foo.getValues(false, true)` doesn't really say what it
does at first glance.

I'll definitely incorporate this!

> 
> a slight tangent thought:
> 
> after looking, it seems we never use(d?) the dirtyOnly
> parameter... so it might be nice to remove that
> 
> we couldn't even have used it anywhere because we always
> set it to false when 'onGetValues' is  a function
> so we'd  need to set it to something different, but
> later we call it unconditionally as a function(?!?)
> 
> but that is rather unrelated, just confused me
> (it seems it was "always" this way, so maybe
> there is some hidden svn history that i'm not seeing
> where the code would make more sense)

I was wondering about that too, but I didn't really want to turn
over too many stones for what's supposed to be just a simple RFC.

I wouldn't mind retiring that parameter for the actual patch series
then, if you'd like. I'll also double-check all call sites to make
sure it's not used anywhere - not that it would make a difference,
but just in case the parameter is provided somewhere.

> 
>> +    getValues: function(dirtyOnly, raw) {
>>       let me = this;
>>         if (Ext.isFunction(me.onGetValues)) {
>> @@ -46,6 +55,10 @@ Ext.define('Proxmox.panel.InputPanel', {
>>           }
>>       });
>>   +    if (raw) {
>> +        return values;
>> +    }
>> +
>>       return me.onGetValues(values);
>>       },
>>   





^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [pve-devel] [RFC pve-manager 1/2] multi disk edit: add copy button
  2023-12-06  9:25   ` Dominik Csapak
@ 2023-12-06 10:49     ` Max Carrara
  0 siblings, 0 replies; 7+ messages in thread
From: Max Carrara @ 2023-12-06 10:49 UTC (permalink / raw)
  To: Dominik Csapak, Proxmox VE development discussion

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





^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-12-06 10:50 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-05 15:44 [pve-devel] [RFC proxmox-widget-toolkit, pve-manager 0/2] Copy Button For Wizard Disk Configuration Max Carrara
2023-12-05 15:44 ` [pve-devel] [RFC proxmox-widget-toolkit 1/2] input panel: add `raw` parameter to function `getValues` Max Carrara
2023-12-06  9:13   ` Dominik Csapak
2023-12-06  9:59     ` Max Carrara
2023-12-05 15:44 ` [pve-devel] [RFC pve-manager 1/2] multi disk edit: add copy button Max Carrara
2023-12-06  9:25   ` Dominik Csapak
2023-12-06 10:49     ` Max Carrara

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal