[pve-devel] [PATCH widget-toolkit 1/2] Utils: add calculate max button width hack

[pve-devel] [PATCH widget-toolkit 1/2] Utils: add calculate max button width hack

[Matthias Heiserer]

The same code is used once in widget toolkit and twice in PVE already,
so it makes sense to add it as a separate function.

Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
---
 src/Utils.js | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/src/Utils.js b/src/Utils.js
index 6a03057..27fcd1d 100644
--- a/src/Utils.js
+++ b/src/Utils.js
@@ -1272,6 +1272,17 @@ utilities: {
 	    .map(val => val.charCodeAt(0)),
 	);
     },
+
+    calculateWidth: function(btn) {
+	// HACK: calculate the max button width on first render to avoid toolbar glitches
+	let defSize = btn.getSize().width;
+
+	btn.setText(btn.altText);
+	let altSize = btn.getSize().width;
+
+	btn.setText(btn.defaultText);
+	btn.setWidth(altSize > defSize ? altSize : defSize);
+    },
 },
 
     singleton: true,
-- 
2.30.2

[pve-devel] [PATCH widget-toolkit 2/2] use the width-helper from utils

[Matthias Heiserer]

Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
---
 src/node/APTRepositories.js | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/src/node/APTRepositories.js b/src/node/APTRepositories.js
index 09ed0be..d63a569 100644
--- a/src/node/APTRepositories.js
+++ b/src/node/APTRepositories.js
@@ -230,16 +230,7 @@ Ext.define('Proxmox.node.APTRepositoriesGrid', {
 		});
 	    },
 	    listeners: {
-		render: function(btn) {
-		    // HACK: calculate the max button width on first render to avoid toolbar glitches
-		    let defSize = btn.getSize().width;
-
-		    btn.setText(btn.altText);
-		    let altSize = btn.getSize().width;
-
-		    btn.setText(btn.defaultText);
-		    btn.setSize({ width: altSize > defSize ? altSize : defSize });
-		},
+		render: Proxmox.Utils.calculateWidth,
 	    },
 	},
     ],
-- 
2.30.2

[pve-devel] [PATCH manager] use the width-helper from utils

[Matthias Heiserer]

Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
---
 www/manager6/lxc/Resources.js     | 14 +-------------
 www/manager6/qemu/HardwareView.js | 14 +-------------
 2 files changed, 2 insertions(+), 26 deletions(-)

diff --git a/www/manager6/lxc/Resources.js b/www/manager6/lxc/Resources.js
index 15ee3c67..1b5d51d9 100644
--- a/www/manager6/lxc/Resources.js
+++ b/www/manager6/lxc/Resources.js
@@ -211,19 +211,7 @@ Ext.define('PVE.lxc.RessourceView', {
 	    },
 	    handler: run_remove,
 	    listeners: {
-		render: function(btn) {
-		    // hack: calculate the max button width on first display to prevent the whole
-		    // toolbar to move when we switch between the "Remove" and "Detach" labels
-		    let def = btn.getSize().width;
-
-		    btn.setText(btn.altText);
-		    let alt = btn.getSize().width;
-
-		    btn.setText(btn.defaultText);
-
-		    let optimal = alt > def ? alt : def;
-		    btn.setSize({ width: optimal });
-		},
+		render: Proxmox.Utils.calculateWidth,
 	    },
 	});
 
diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
index 6cea4287..473cfcf4 100644
--- a/www/manager6/qemu/HardwareView.js
+++ b/www/manager6/qemu/HardwareView.js
@@ -486,19 +486,7 @@ Ext.define('PVE.qemu.HardwareView', {
 		});
 	    },
 	    listeners: {
-		render: function(btn) {
-		    // hack: calculate the max button width on first display to prevent the whole
-		    // toolbar to move when we switch between the "Remove" and "Detach" labels
-		    var def = btn.getSize().width;
-
-		    btn.setText(btn.altText);
-		    var alt = btn.getSize().width;
-
-		    btn.setText(btn.defaultText);
-
-		    var optimal = alt > def ? alt : def;
-		    btn.setSize({ width: optimal });
-		},
+		render: Proxmox.Utils.calculateWidth,
 	    },
 	});
 
-- 
2.30.2

Re: [pve-devel] [PATCH widget-toolkit 1/2] Utils: add calculate max button width hack

[Thomas Lamprecht]

On 24.03.22 12:33, Matthias Heiserer wrote:
> The same code is used once in widget toolkit and twice in PVE already,
> so it makes sense to add it as a separate function.
> 
> Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
> ---
>  src/Utils.js | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/src/Utils.js b/src/Utils.js
> index 6a03057..27fcd1d 100644
> --- a/src/Utils.js
> +++ b/src/Utils.js
> @@ -1272,6 +1272,17 @@ utilities: {
>  	    .map(val => val.charCodeAt(0)),
>  	);
>      },
> +
> +    calculateWidth: function(btn) {

way to general name for such a specific use case, NACK, rather:

maxAltTextButtonWidth

> +	// HACK: calculate the max button width on first render to avoid toolbar glitches

comment could go in line above the function declaration.

> +	let defSize = btn.getSize().width;

now that we're not on a deeper indentation level we can also spell out the variable
names more nicely.

> +
> +	btn.setText(btn.altText);
> +	let altSize = btn.getSize().width;
> +
> +	btn.setText(btn.defaultText);
> +	btn.setWidth(altSize > defSize ? altSize : defSize);
> +    },
>  },
>  
>      singleton: true,

Re: [pve-devel] [PATCH widget-toolkit 2/2] use the width-helper from utils

[Thomas Lamprecht]

please add a prefixing tag to the comment subject:

apt repos: use new button max-width helper from utils

On 24.03.22 12:33, Matthias Heiserer wrote:
> Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
> ---
>  src/node/APTRepositories.js | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/src/node/APTRepositories.js b/src/node/APTRepositories.js
> index 09ed0be..d63a569 100644
> --- a/src/node/APTRepositories.js
> +++ b/src/node/APTRepositories.js
> @@ -230,16 +230,7 @@ Ext.define('Proxmox.node.APTRepositoriesGrid', {
>  		});
>  	    },
>  	    listeners: {
> -		render: function(btn) {
> -		    // HACK: calculate the max button width on first render to avoid toolbar glitches
> -		    let defSize = btn.getSize().width;
> -
> -		    btn.setText(btn.altText);
> -		    let altSize = btn.getSize().width;
> -
> -		    btn.setText(btn.defaultText);
> -		    btn.setSize({ width: altSize > defSize ? altSize : defSize });
> -		},
> +		render: Proxmox.Utils.calculateWidth,
>  	    },
>  	},
>      ],

Re: [pve-devel] [PATCH widget-toolkit 1/2] Utils: add calculate max button width hack

[Thomas Lamprecht]

On 24.03.22 12:33, Matthias Heiserer wrote:
> The same code is used once in widget toolkit and twice in PVE already,
> so it makes sense to add it as a separate function.
> 

FWIW, there'd be also the possibility of adding a new button class, derived
from proxmoxButton, that would be a bit more boilerplate but also avoid having a
rather specific helper in the general utils class and avoid coupling from property
existance/behavior over such "code distance". Maybe we could also move some more
common "AltButton" behavior in there though (did not checked too closely).
What do you think?

Also, it could be great to avoid the "text at call time is the initial text"
assumption in the width calculation, e.g.,

let otherText = btn.text === btn.defaultText ? btn.altText : btn.defaultText;

currently unnecessary, but would make it more robust and to easy to not do, IMO.

Re: [pve-devel] [PATCH manager] use the width-helper from utils

[Thomas Lamprecht]

as a v2 is required anyway, please adapt the subject:

"ui: tree-wide: use new button max-width helper from utils"

Re: [pve-devel] [PATCH widget-toolkit 1/2] Utils: add calculate max button width hack

[Matthias Heiserer]

On 25.03.2022 09:47, Thomas Lamprecht wrote:
> On 24.03.22 12:33, Matthias Heiserer wrote:
>> The same code is used once in widget toolkit and twice in PVE already,
>> so it makes sense to add it as a separate function.
>>
> 
> FWIW, there'd be also the possibility of adding a new button class, derived
> from proxmoxButton, that would be a bit more boilerplate but also avoid having a
> rather specific helper in the general utils class and avoid coupling from property
> existance/behavior over such "code distance". Maybe we could also move some more
> common "AltButton" behavior in there though (did not checked too closely).
> What do you think?
> 
Not a big fan tbh, as would mean it can't be used with an Ext.button.
Id prefer putting the helper in the Button file, but that doesn't seem 
to be possible with Extjs.

> Also, it could be great to avoid the "text at call time is the initial text"
> assumption in the width calculation, e.g.,
> 
> let otherText = btn.text === btn.defaultText ? btn.altText : btn.defaultText;
> 
> currently unnecessary, but would make it more robust and to easy to not do, IMO.

good call

Re: [pve-devel] [PATCH widget-toolkit 1/2] Utils: add calculate max button width hack

[Thomas Lamprecht]

On 28.03.22 11:43, Matthias Heiserer wrote:
> On 25.03.2022 09:47, Thomas Lamprecht wrote:
>> On 24.03.22 12:33, Matthias Heiserer wrote:
>>> The same code is used once in widget toolkit and twice in PVE already,
>>> so it makes sense to add it as a separate function.
>>>
>>
>> FWIW, there'd be also the possibility of adding a new button class, derived
>> from proxmoxButton, that would be a bit more boilerplate but also avoid having a
>> rather specific helper in the general utils class and avoid coupling from property
>> existance/behavior over such "code distance". Maybe we could also move some more
>> common "AltButton" behavior in there though (did not checked too closely).
>> What do you think?
>>
> Not a big fan tbh, as would mean it can't be used with an Ext.button.

Not seeing the problem? You just use the derived xtype which *is* a Ext.button,
we alreay use an also derived variant in the respective places anyway (proxmoxButton)
so not sure which problems you envision...

Re: [pve-devel] [PATCH widget-toolkit 1/2] Utils: add calculate max button width hack

[Matthias Heiserer]

On 28.03.2022 11:44, Thomas Lamprecht wrote:
> On 28.03.22 11:43, Matthias Heiserer wrote:
>> On 25.03.2022 09:47, Thomas Lamprecht wrote:
>>> On 24.03.22 12:33, Matthias Heiserer wrote:
>>>> The same code is used once in widget toolkit and twice in PVE already,
>>>> so it makes sense to add it as a separate function.
>>>>
>>>
>>> FWIW, there'd be also the possibility of adding a new button class, derived
>>> from proxmoxButton, that would be a bit more boilerplate but also avoid having a
>>> rather specific helper in the general utils class and avoid coupling from property
>>> existance/behavior over such "code distance". Maybe we could also move some more
>>> common "AltButton" behavior in there though (did not checked too closely).
>>> What do you think?
>>>
>> Not a big fan tbh, as would mean it can't be used with an Ext.button.
> 
> Not seeing the problem? You just use the derived xtype which *is* a Ext.button,
> we alreay use an also derived variant in the respective places anyway (proxmoxButton)
> so not sure which problems you envision...
I meant in case we need the functionality with an ext button, but as you 
say, can just use a proxmoxButton instead then.
Will send a v2 with a separate class. You want it named AltButton?

Re: [pve-devel] [PATCH widget-toolkit 1/2] Utils: add calculate max button width hack

[Thomas Lamprecht]

On 28.03.22 12:17, Matthias Heiserer wrote:
> On 28.03.2022 11:44, Thomas Lamprecht wrote:
>> On 28.03.22 11:43, Matthias Heiserer wrote:
>>> On 25.03.2022 09:47, Thomas Lamprecht wrote:
>>>> On 24.03.22 12:33, Matthias Heiserer wrote:
>>>>> The same code is used once in widget toolkit and twice in PVE already,
>>>>> so it makes sense to add it as a separate function.
>>>>>
>>>>
>>>> FWIW, there'd be also the possibility of adding a new button class, derived
>>>> from proxmoxButton, that would be a bit more boilerplate but also avoid having a
>>>> rather specific helper in the general utils class and avoid coupling from property
>>>> existance/behavior over such "code distance". Maybe we could also move some more
>>>> common "AltButton" behavior in there though (did not checked too closely).
>>>> What do you think?
>>>>
>>> Not a big fan tbh, as would mean it can't be used with an Ext.button.
>>
>> Not seeing the problem? You just use the derived xtype which *is* a Ext.button,
>> we alreay use an also derived variant in the respective places anyway (proxmoxButton)
>> so not sure which problems you envision...
>
> I meant in case we need the functionality with an ext button, but as you say, can just use a proxmoxButton instead then.

Still not get the issue? As the new button is a descendant of the Ext.button it
can be switched out transparently, so in that case you just use the new button
class, I mean that's the whole point..

> Will send a v2 with a separate class. You want it named AltButton?

I'd go for:

Ext.define('Proxmox.button.AltText', {
     extend: 'Proxmox.button.Button',
     xtype: 'proxmoxAltTextButton',

    ...
});