[pve-devel] [PATCH widget-toolkit/manager] improve combogrid default value handling

[pve-devel] [PATCH widget-toolkit/manager] improve combogrid default value handling

[Dominik Csapak]

the default value for combogrids is '', but we often need to set it to
[], to avoid issues with dirty tracking. Fix this by setting it to [] by
default, making it unnecessary to carry the workaround + comment around
in child classes.

the first patch of pve-manager is a bit unrelated but popped up during
development (can be applied independently)

the second patch of pve-manager is also a bit independent, but it fixes
wrong dirty tracking in the firewall rule edit window iff the widget
toolkit patch is also present

the third manager patch depends on the widget toolkit patch

proxmox-widget-toolkit:

Dominik Csapak (1):
  combogrid: initialze value with [] by default

 src/form/ComboGrid.js       | 2 +-
 src/form/NetworkSelector.js | 4 ----
 2 files changed, 1 insertion(+), 5 deletions(-)

pve-manager:

Dominik Csapak (3):
  ui: ipset: make ip/cidr required
  ui: don't set the default value of combogrids to ''
  ui: don't set the default value of combogrids to []

 www/manager6/form/NodeSelector.js  | 5 +----
 www/manager6/grid/FirewallRules.js | 2 --
 www/manager6/ha/GroupSelector.js   | 1 -
 www/manager6/panel/IPSet.js        | 2 +-
 4 files changed, 2 insertions(+), 8 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH widget-toolkit 1/1] combogrid: initialze value with [] by default

[Dominik Csapak]

we have to initialize the value of a combogrid to something (else extjs
does not initialize everything in the object *sometimes* for yet unknown
reasons), but the empty string is wrong.

we already have at least two places where we set the default value to []
(namely NodeSelector and ha GroupSelector) with the comment:

 // set default value to empty array, else it inits it with
 // null and after the store load it is an empty array,
 // triggering dirtychange

so it makes sense to always set it to that by default. This only ever is
relevant when the combogrid has `allowBlank: true`, since if it does not
it's either invalid (and thus "dirty") or it has a selected value anyway

this should make the manual setting of

 value: [],

unnecessary in the child classes. We can even remove it direcly in the
NetworkSelector.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/form/ComboGrid.js       | 2 +-
 src/form/NetworkSelector.js | 4 ----
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/src/form/ComboGrid.js b/src/form/ComboGrid.js
index 55dee7e..6338a3a 100644
--- a/src/form/ComboGrid.js
+++ b/src/form/ComboGrid.js
@@ -400,7 +400,7 @@ Ext.define('Proxmox.form.ComboGrid', {
 	    matchFieldWidth: false,
 	});
 
-	Ext.applyIf(me, { value: '' }); // hack: avoid ExtJS validate() bug
+	Ext.applyIf(me, { value: [] }); // hack: avoid ExtJS validate() bug
 
 	Ext.applyIf(me.listConfig, { width: 400 });
 
diff --git a/src/form/NetworkSelector.js b/src/form/NetworkSelector.js
index 86d394d..30a1cd4 100644
--- a/src/form/NetworkSelector.js
+++ b/src/form/NetworkSelector.js
@@ -45,10 +45,6 @@ Ext.define('Proxmox.form.NetworkSelector', {
 	    networkSelectorStore.load();
 	}
     },
-    // set default value to empty array, else it inits it with
-    // null and after the store load it is an empty array,
-    // triggering dirtychange
-    value: [],
     valueField: 'cidr',
     displayField: 'cidr',
     store: {
-- 
2.30.2

[pve-devel] [PATCH manager 1/3] ui: ipset: make ip/cidr required

[Dominik Csapak]

it is in the backend, so make it required in the gui too

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
can be applied independently

 www/manager6/panel/IPSet.js | 1 +
 1 file changed, 1 insertion(+)

diff --git a/www/manager6/panel/IPSet.js b/www/manager6/panel/IPSet.js
index c449cdaa..d42d062d 100644
--- a/www/manager6/panel/IPSet.js
+++ b/www/manager6/panel/IPSet.js
@@ -203,6 +203,7 @@ Ext.define('PVE.IPSetCidrEdit', {
 		editable: true,
 		base_url: me.list_refs_url,
 		value: '',
+		allowBlank: false,
 		fieldLabel: gettext('IP/CIDR'),
 	    });
 	} else {
-- 
2.30.2

[pve-devel] [PATCH manager 2/3] ui: don't set the default value of combogrids to ''

[Dominik Csapak]

the combogrid does that itself already

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
can be applied independently, but fixes dirty tracking when the
widget-toolkit patch is also present

 www/manager6/grid/FirewallRules.js | 2 --
 www/manager6/panel/IPSet.js        | 1 -
 2 files changed, 3 deletions(-)

diff --git a/www/manager6/grid/FirewallRules.js b/www/manager6/grid/FirewallRules.js
index 18075eaa..11881bf7 100644
--- a/www/manager6/grid/FirewallRules.js
+++ b/www/manager6/grid/FirewallRules.js
@@ -234,7 +234,6 @@ Ext.define('PVE.FirewallRulePanel', {
 		autoSelect: false,
 		editable: true,
 		base_url: me.list_refs_url,
-		value: '',
 		fieldLabel: gettext('Source'),
 		maxLength: 512,
 		maxLengthText: gettext('Too long, consider using IP sets.'),
@@ -245,7 +244,6 @@ Ext.define('PVE.FirewallRulePanel', {
 		autoSelect: false,
 		editable: true,
 		base_url: me.list_refs_url,
-		value: '',
 		fieldLabel: gettext('Destination'),
 		maxLength: 512,
 		maxLengthText: gettext('Too long, consider using IP sets.'),
diff --git a/www/manager6/panel/IPSet.js b/www/manager6/panel/IPSet.js
index d42d062d..d96ed18a 100644
--- a/www/manager6/panel/IPSet.js
+++ b/www/manager6/panel/IPSet.js
@@ -202,7 +202,6 @@ Ext.define('PVE.IPSetCidrEdit', {
 		autoSelect: false,
 		editable: true,
 		base_url: me.list_refs_url,
-		value: '',
 		allowBlank: false,
 		fieldLabel: gettext('IP/CIDR'),
 	    });
-- 
2.30.2

[pve-devel] [PATCH manager 3/3] ui: don't set the default value of combogrids to []

[Dominik Csapak]

the combogrid sets the default itself correctly

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
depends on the widget-toolkit patch

 www/manager6/form/NodeSelector.js | 5 +----
 www/manager6/ha/GroupSelector.js  | 1 -
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/www/manager6/form/NodeSelector.js b/www/manager6/form/NodeSelector.js
index c3e3da31..bc282287 100644
--- a/www/manager6/form/NodeSelector.js
+++ b/www/manager6/form/NodeSelector.js
@@ -12,10 +12,7 @@ Ext.define('PVE.form.NodeSelector', {
 
     // only allow those nodes (array)
     allowedNodes: undefined,
-    // set default value to empty array, else it inits it with
-    // null and after the store load it is an empty array,
-    // triggering dirtychange
-    value: [],
+
     valueField: 'node',
     displayField: 'node',
     store: {
diff --git a/www/manager6/ha/GroupSelector.js b/www/manager6/ha/GroupSelector.js
index 61ab0c03..1823472f 100644
--- a/www/manager6/ha/GroupSelector.js
+++ b/www/manager6/ha/GroupSelector.js
@@ -2,7 +2,6 @@ Ext.define('PVE.ha.GroupSelector', {
     extend: 'Proxmox.form.ComboGrid',
     alias: ['widget.pveHAGroupSelector'],
 
-    value: [],
     autoSelect: false,
     valueField: 'group',
     displayField: 'group',
-- 
2.30.2

Re: [pve-devel] [PATCH widget-toolkit/manager] improve combogrid default value handling

[Fabian Grünbichler]

somewhat plays into #4840 , in that we now no longer do a bogus GET call
for the 'null' storage and display a misleading permission error as a
result, but there still could be a nice (big?) error message about the
general lack of (accessible) storages ;)

https://bugzilla.proxmox.com/show_bug.cgi?id=4840

On July 19, 2023 2:11 pm, Dominik Csapak wrote:
> the default value for combogrids is '', but we often need to set it to
> [], to avoid issues with dirty tracking. Fix this by setting it to [] by
> default, making it unnecessary to carry the workaround + comment around
> in child classes.
> 
> the first patch of pve-manager is a bit unrelated but popped up during
> development (can be applied independently)
> 
> the second patch of pve-manager is also a bit independent, but it fixes
> wrong dirty tracking in the firewall rule edit window iff the widget
> toolkit patch is also present
> 
> the third manager patch depends on the widget toolkit patch
> 
> proxmox-widget-toolkit:
> 
> Dominik Csapak (1):
>   combogrid: initialze value with [] by default
> 
>  src/form/ComboGrid.js       | 2 +-
>  src/form/NetworkSelector.js | 4 ----
>  2 files changed, 1 insertion(+), 5 deletions(-)
> 
> pve-manager:
> 
> Dominik Csapak (3):
>   ui: ipset: make ip/cidr required
>   ui: don't set the default value of combogrids to ''
>   ui: don't set the default value of combogrids to []
> 
>  www/manager6/form/NodeSelector.js  | 5 +----
>  www/manager6/grid/FirewallRules.js | 2 --
>  www/manager6/ha/GroupSelector.js   | 1 -
>  www/manager6/panel/IPSet.js        | 2 +-
>  4 files changed, 2 insertions(+), 8 deletions(-)
> 
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

[pve-devel] applied: [PATCH widget-toolkit 1/1] combogrid: initialze value with [] by default

[Thomas Lamprecht]

Am 19/07/2023 um 14:11 schrieb Dominik Csapak:
> we have to initialize the value of a combogrid to something (else extjs
> does not initialize everything in the object *sometimes* for yet unknown
> reasons), but the empty string is wrong.
> 
> we already have at least two places where we set the default value to []
> (namely NodeSelector and ha GroupSelector) with the comment:
> 
>  // set default value to empty array, else it inits it with
>  // null and after the store load it is an empty array,
>  // triggering dirtychange
> 
> so it makes sense to always set it to that by default. This only ever is
> relevant when the combogrid has `allowBlank: true`, since if it does not
> it's either invalid (and thus "dirty") or it has a selected value anyway
> 
> this should make the manual setting of
> 
>  value: [],
> 
> unnecessary in the child classes. We can even remove it direcly in the
> NetworkSelector.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/form/ComboGrid.js       | 2 +-
>  src/form/NetworkSelector.js | 4 ----
>  2 files changed, 1 insertion(+), 5 deletions(-)
> 
>

applied, thanks!

[pve-devel] applied-series: [PATCH widget-toolkit/manager] improve combogrid default value handling

[Thomas Lamprecht]

Am 19/07/2023 um 14:11 schrieb Dominik Csapak:
> pve-manager:
> 
> Dominik Csapak (3):
>   ui: ipset: make ip/cidr required
>   ui: don't set the default value of combogrids to ''
>   ui: don't set the default value of combogrids to []
> 

applied those now too, thanks!