* [pve-devel] [PATCH manager] ui: lxc options: fix rendering default and '0' value for tty count @ 2025-11-17 9:21 Dominik Csapak 2025-11-17 10:41 ` Thomas Lamprecht 2025-11-17 11:24 ` [pve-devel] superseded: " Dominik Csapak 0 siblings, 2 replies; 4+ messages in thread From: Dominik Csapak @ 2025-11-17 9:21 UTC (permalink / raw) To: pve-devel if '0' was entered, the options grid would show an empty line. Fix that by adding a renderer that checks if the value is a number. If the default is set, the edit window would always pre-fill the value '2' instead of leaving the field empty. Fix that by removing the manually set 'value' property. Reported-by: Dietmar Maurer <dietmar@proxmox.com> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com> --- www/manager6/lxc/Options.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/www/manager6/lxc/Options.js b/www/manager6/lxc/Options.js index 6cc2e2b4..0acb88f4 100644 --- a/www/manager6/lxc/Options.js +++ b/www/manager6/lxc/Options.js @@ -81,6 +81,7 @@ Ext.define('PVE.lxc.Options', { tty: { header: gettext('TTY count'), defaultValue: 2, + renderer: (value) => (Ext.isNumber(value) ? value : '2'), editor: caps.vms['VM.Config.Options'] ? { xtype: 'proxmoxWindowEdit', @@ -90,7 +91,6 @@ Ext.define('PVE.lxc.Options', { name: 'tty', minValue: 0, maxValue: 6, - value: 2, fieldLabel: gettext('TTY count'), emptyText: gettext('Default'), deleteEmpty: true, -- 2.47.3 _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH manager] ui: lxc options: fix rendering default and '0' value for tty count 2025-11-17 9:21 [pve-devel] [PATCH manager] ui: lxc options: fix rendering default and '0' value for tty count Dominik Csapak @ 2025-11-17 10:41 ` Thomas Lamprecht 2025-11-17 11:09 ` Dominik Csapak 2025-11-17 11:24 ` [pve-devel] superseded: " Dominik Csapak 1 sibling, 1 reply; 4+ messages in thread From: Thomas Lamprecht @ 2025-11-17 10:41 UTC (permalink / raw) To: Proxmox VE development discussion, Dominik Csapak Am 17.11.25 um 10:21 schrieb Dominik Csapak: > if '0' was entered, the options grid would show an empty line. Fix that > by adding a renderer that checks if the value is a number. I was a bit confused by this, as it sounded like setting it to '0' in the frontend caused some non-numeric value in the backend which then was handled wrong here. But it's really just the default renderer in PendingObjectGrid's renderValue using a truthy fallback to empty string for the value, i.e. `value || ''` Nowadays we could replace the boolean or `||` with a nullish coalescing operator `??`, which is limited to fallback only if the value is exactly one of `null` or `undefined`, that should also fix these issues in a more generic fashion. > > If the default is set, the edit window would always pre-fill the value > '2' instead of leaving the field empty. Fix that by removing the > manually set 'value' property. > > Reported-by: Dietmar Maurer <dietmar@proxmox.com> > Signed-off-by: Dominik Csapak <d.csapak@proxmox.com> > --- > www/manager6/lxc/Options.js | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/www/manager6/lxc/Options.js b/www/manager6/lxc/Options.js > index 6cc2e2b4..0acb88f4 100644 > --- a/www/manager6/lxc/Options.js > +++ b/www/manager6/lxc/Options.js > @@ -81,6 +81,7 @@ Ext.define('PVE.lxc.Options', { > tty: { > header: gettext('TTY count'), > defaultValue: 2, > + renderer: (value) => (Ext.isNumber(value) ? value : '2'), But this would mask any other non-numeric value with "2" too, and then be potentially misleading. Initially thought this was more theoretic, but Ext.isNumeric basically comes down to `typeof value === "number"', so if anything in the perl backend just "looks" at the returned value the wrong way and it gets stringified along the way, we would mask the underlying value here. In Yew based UIs being fully typed is OK as there deserialization fails explicitly and one has to chose the behavior for how one wants to handle this, but in JS we (sadly) cannot really relay on this. > editor: caps.vms['VM.Config.Options'] > ? { > xtype: 'proxmoxWindowEdit', > @@ -90,7 +91,6 @@ Ext.define('PVE.lxc.Options', { > name: 'tty', > minValue: 0, > maxValue: 6, > - value: 2, > fieldLabel: gettext('TTY count'), > emptyText: gettext('Default'), > deleteEmpty: true, _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH manager] ui: lxc options: fix rendering default and '0' value for tty count 2025-11-17 10:41 ` Thomas Lamprecht @ 2025-11-17 11:09 ` Dominik Csapak 0 siblings, 0 replies; 4+ messages in thread From: Dominik Csapak @ 2025-11-17 11:09 UTC (permalink / raw) To: Thomas Lamprecht, Proxmox VE development discussion On 11/17/25 11:41 AM, Thomas Lamprecht wrote: > Am 17.11.25 um 10:21 schrieb Dominik Csapak: >> if '0' was entered, the options grid would show an empty line. Fix that >> by adding a renderer that checks if the value is a number. > > I was a bit confused by this, as it sounded like setting it to '0' in the > frontend caused some non-numeric value in the backend which then was handled > wrong here. > > But it's really just the default renderer in PendingObjectGrid's renderValue > using a truthy fallback to empty string for the value, i.e. `value || ''` > > Nowadays we could replace the boolean or `||` with a nullish coalescing > operator `??`, which is limited to fallback only if the value is exactly > one of `null` or `undefined`, that should also fix these issues in a more > generic fashion. > true, yeah i'll send a separate patch for that, then the first part here shouldn't be necessary at all. >> >> If the default is set, the edit window would always pre-fill the value >> '2' instead of leaving the field empty. Fix that by removing the >> manually set 'value' property. >> >> Reported-by: Dietmar Maurer <dietmar@proxmox.com> >> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com> >> --- >> www/manager6/lxc/Options.js | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/www/manager6/lxc/Options.js b/www/manager6/lxc/Options.js >> index 6cc2e2b4..0acb88f4 100644 >> --- a/www/manager6/lxc/Options.js >> +++ b/www/manager6/lxc/Options.js >> @@ -81,6 +81,7 @@ Ext.define('PVE.lxc.Options', { >> tty: { >> header: gettext('TTY count'), >> defaultValue: 2, >> + renderer: (value) => (Ext.isNumber(value) ? value : '2'), > > But this would mask any other non-numeric value with "2" too, and then be > potentially misleading. Initially thought this was more theoretic, but > Ext.isNumeric basically comes down to `typeof value === "number"', so if > anything in the perl backend just "looks" at the returned value the wrong > way and it gets stringified along the way, we would mask the underlying > value here. caution there are two methods in extjs here: isNumber (which i used and has the properties you described) and isNumeric, which actually tries to parse the value and checks if it's a number (not NaN), so that would have worked out better (but still unnecessary if the pending object grid is fixed) > > In Yew based UIs being fully typed is OK as there deserialization fails > explicitly and one has to chose the behavior for how one wants to handle > this, but in JS we (sadly) cannot really relay on this. > > >> editor: caps.vms['VM.Config.Options'] >> ? { >> xtype: 'proxmoxWindowEdit', >> @@ -90,7 +91,6 @@ Ext.define('PVE.lxc.Options', { >> name: 'tty', >> minValue: 0, >> maxValue: 6, >> - value: 2, >> fieldLabel: gettext('TTY count'), >> emptyText: gettext('Default'), >> deleteEmpty: true, > _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
* [pve-devel] superseded: [PATCH manager] ui: lxc options: fix rendering default and '0' value for tty count 2025-11-17 9:21 [pve-devel] [PATCH manager] ui: lxc options: fix rendering default and '0' value for tty count Dominik Csapak 2025-11-17 10:41 ` Thomas Lamprecht @ 2025-11-17 11:24 ` Dominik Csapak 1 sibling, 0 replies; 4+ messages in thread From: Dominik Csapak @ 2025-11-17 11:24 UTC (permalink / raw) To: pve-devel superseded by two patches: https://lore.proxmox.com/pve-devel/20251117092110.628324-1-d.csapak@proxmox.com/ https://lore.proxmox.com/pve-devel/20251117112334.1400372-1-d.csapak@proxmox.com/ On 11/17/25 10:21 AM, Dominik Csapak wrote: > if '0' was entered, the options grid would show an empty line. Fix that > by adding a renderer that checks if the value is a number. > > If the default is set, the edit window would always pre-fill the value > '2' instead of leaving the field empty. Fix that by removing the > manually set 'value' property. > > Reported-by: Dietmar Maurer <dietmar@proxmox.com> > Signed-off-by: Dominik Csapak <d.csapak@proxmox.com> > --- > www/manager6/lxc/Options.js | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/www/manager6/lxc/Options.js b/www/manager6/lxc/Options.js > index 6cc2e2b4..0acb88f4 100644 > --- a/www/manager6/lxc/Options.js > +++ b/www/manager6/lxc/Options.js > @@ -81,6 +81,7 @@ Ext.define('PVE.lxc.Options', { > tty: { > header: gettext('TTY count'), > defaultValue: 2, > + renderer: (value) => (Ext.isNumber(value) ? value : '2'), > editor: caps.vms['VM.Config.Options'] > ? { > xtype: 'proxmoxWindowEdit', > @@ -90,7 +91,6 @@ Ext.define('PVE.lxc.Options', { > name: 'tty', > minValue: 0, > maxValue: 6, > - value: 2, > fieldLabel: gettext('TTY count'), > emptyText: gettext('Default'), > deleteEmpty: true, _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-11-17 11:24 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2025-11-17 9:21 [pve-devel] [PATCH manager] ui: lxc options: fix rendering default and '0' value for tty count Dominik Csapak 2025-11-17 10:41 ` Thomas Lamprecht 2025-11-17 11:09 ` Dominik Csapak 2025-11-17 11:24 ` [pve-devel] superseded: " Dominik Csapak
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.