[pve-devel] [PATCH manager v2] fix #4758: ui: lxc wizard: allow multiple ssh keys

[pve-devel] [PATCH manager v2] fix #4758: ui: lxc wizard: allow multiple ssh keys

[Dominik Csapak]

by converting the textfield into a textarea and validate the value
line wise (if there is more than one line)

also create a 'MultiFileButton' (mostly copied from extjs) that allows
to select multiple files at once

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v2:
* reduces lines of code (Thanks @Thomas)
* implemented multi file select

 www/manager6/Makefile                |  1 +
 www/manager6/form/MultiFileButton.js | 58 ++++++++++++++++++++++++++++
 www/manager6/lxc/CreateWizard.js     | 19 +++++----
 3 files changed, 71 insertions(+), 7 deletions(-)
 create mode 100644 www/manager6/form/MultiFileButton.js

diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index 5b455c80..7ec9d7a5 100644
--- a/www/manager6/Makefile
+++ b/www/manager6/Makefile
@@ -84,6 +84,7 @@ JSSRC= 							\
 	form/ListField.js				\
 	form/Tag.js					\
 	form/TagEdit.js					\
+	form/MultiFileButton.js				\
 	grid/BackupView.js				\
 	grid/FirewallAliases.js				\
 	grid/FirewallOptions.js				\
diff --git a/www/manager6/form/MultiFileButton.js b/www/manager6/form/MultiFileButton.js
new file mode 100644
index 00000000..a73662f2
--- /dev/null
+++ b/www/manager6/form/MultiFileButton.js
@@ -0,0 +1,58 @@
+// mostly copied from ExtJS FileButton, but added 'multiple' at the relevant
+// places so we have a file picker where one can select multiple files
+Ext.define('PVE.form.MultiFileButton', {
+    extend: 'Ext.form.field.FileButton',
+    alias: 'widget.pveMultiFileButton',
+
+    afterTpl: [
+	'<input id="{id}-fileInputEl" data-ref="fileInputEl" class="{childElCls} {inputCls}" ',
+	    'type="file" size="1" name="{inputName}" unselectable="on" multiple ',
+	    '<tpl if="accept != null">accept="{accept}"</tpl>',
+	    '<tpl if="tabIndex != null">tabindex="{tabIndex}"</tpl>',
+	'>',
+    ],
+
+    createFileInput: function(isTemporary) {
+	var me = this,
+	    fileInputEl, listeners;
+
+	fileInputEl = me.fileInputEl = me.el.createChild({
+	    name: me.inputName || me.id,
+	    multiple: true,
+	    id: !isTemporary ? me.id + '-fileInputEl' : undefined,
+	    cls: me.inputCls + (me.getInherited().rtl ? ' ' + Ext.baseCSSPrefix + 'rtl' : ''),
+	    tag: 'input',
+	    type: 'file',
+	    size: 1,
+	    unselectable: 'on',
+	}, me.afterInputGuard); // Nothing special happens outside of IE/Edge
+
+	// This is our focusEl
+	fileInputEl.dom.setAttribute('data-componentid', me.id);
+
+	if (me.tabIndex !== null) {
+	    me.setTabIndex(me.tabIndex);
+	}
+
+	if (me.accept) {
+	    fileInputEl.dom.setAttribute('accept', me.accept);
+	}
+
+	// We place focus and blur listeners on fileInputEl to activate Button's
+	// focus and blur style treatment
+	listeners = {
+	    scope: me,
+	    change: me.fireChange,
+	    mousedown: me.handlePrompt,
+	    keydown: me.handlePrompt,
+	    focus: me.onFileFocus,
+	    blur: me.onFileBlur,
+	};
+
+	if (me.useTabGuards) {
+	    listeners.keydown = me.onFileInputKeydown;
+	}
+
+	fileInputEl.on(listeners);
+    },
+});
diff --git a/www/manager6/lxc/CreateWizard.js b/www/manager6/lxc/CreateWizard.js
index 0b82cc1c..7cc59299 100644
--- a/www/manager6/lxc/CreateWizard.js
+++ b/www/manager6/lxc/CreateWizard.js
@@ -120,16 +120,16 @@ Ext.define('PVE.lxc.CreateWizard', {
 		    },
 		},
 		{
-		    xtype: 'proxmoxtextfield',
+		    xtype: 'textarea',
 		    name: 'ssh-public-keys',
 		    value: '',
-		    fieldLabel: gettext('SSH public key'),
+		    fieldLabel: gettext('SSH public key(s)'),
 		    allowBlank: true,
 		    validator: function(value) {
 			let pwfield = this.up().down('field[name=password]');
 			if (value.length) {
-			    let key = PVE.Parser.parseSSHKey(value);
-			    if (!key) {
+			    let keys = value.indexOf('\n') !== -1 ? value.split('\n') : [ value ];
+			    if (keys.some(key => key === '' || !PVE.Parser.parseSSHKey(key))) {
 				return "Failed to recognize ssh key";
 			    }
 			    pwfield.allowBlank = true;
@@ -159,15 +159,20 @@ Ext.define('PVE.lxc.CreateWizard', {
 		    },
 		},
 		{
-		    xtype: 'filebutton',
+		    xtype: 'pveMultiFileButton',
 		    name: 'file',
 		    hidden: !window.FileReader,
 		    text: gettext('Load SSH Key File'),
 		    listeners: {
 			change: function(btn, e, value) {
 			    e = e.event;
-			    let field = this.up().down('proxmoxtextfield[name=ssh-public-keys]');
-			    PVE.Utils.loadSSHKeyFromFile(e.target.files[0], v => field.setValue(v));
+			    let field = this.up().down('textarea[name=ssh-public-keys]');
+			    for (const file of e?.target?.files ?? []) {
+				PVE.Utils.loadSSHKeyFromFile(file, v => {
+				    let oldValue = field.getValue();
+				    field.setValue(oldValue ? `${oldValue}\n${v.trim()}` : v.trim());
+				});
+			    }
 			    btn.reset();
 			},
 		    },
-- 
2.30.2

Re: [pve-devel] [PATCH manager v2] fix #4758: ui: lxc wizard: allow multiple ssh keys

[Christoph Heiss]

On Mon, Jul 03, 2023 at 04:51:16PM +0200, Dominik Csapak wrote:
> by converting the textfield into a textarea and validate the value
> line wise (if there is more than one line)
>
> also create a 'MultiFileButton' (mostly copied from extjs) that allows
> to select multiple files at once
>

Some small nits inline, but otherwise:

Tested-by: Christoph Heiss <c.heiss@proxmox.com>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> changes from v2:
> * reduces lines of code (Thanks @Thomas)
> * implemented multi file select
>
>  www/manager6/Makefile                |  1 +
>  www/manager6/form/MultiFileButton.js | 58 ++++++++++++++++++++++++++++
>  www/manager6/lxc/CreateWizard.js     | 19 +++++----
>  3 files changed, 71 insertions(+), 7 deletions(-)
>  create mode 100644 www/manager6/form/MultiFileButton.js
>
> [..]
> diff --git a/www/manager6/form/MultiFileButton.js b/www/manager6/form/MultiFileButton.js
> new file mode 100644
> index 00000000..a73662f2
> --- /dev/null
> +++ b/www/manager6/form/MultiFileButton.js
> @@ -0,0 +1,58 @@
> +// mostly copied from ExtJS FileButton, but added 'multiple' at the relevant
> +// places so we have a file picker where one can select multiple files
> +Ext.define('PVE.form.MultiFileButton', {
> +    extend: 'Ext.form.field.FileButton',
> +    alias: 'widget.pveMultiFileButton',
> +
> +    afterTpl: [
> +	'<input id="{id}-fileInputEl" data-ref="fileInputEl" class="{childElCls} {inputCls}" ',
> +	    'type="file" size="1" name="{inputName}" unselectable="on" multiple ',
> +	    '<tpl if="accept != null">accept="{accept}"</tpl>',
> +	    '<tpl if="tabIndex != null">tabindex="{tabIndex}"</tpl>',
> +	'>',
> +    ],
> +
> +    createFileInput: function(isTemporary) {
> +	var me = this,
nit: s/var/let/

> +	    fileInputEl, listeners;
> +
> +	fileInputEl = me.fileInputEl = me.el.createChild({
nit: Is `me.fileInputEl` used somewhere I'm not seeing? Otherwise, could
be eliminated and just be `let fileInputEl = ..`.

> +	    name: me.inputName || me.id,
> +	    multiple: true,
> +	    id: !isTemporary ? me.id + '-fileInputEl' : undefined,
> +	    cls: me.inputCls + (me.getInherited().rtl ? ' ' + Ext.baseCSSPrefix + 'rtl' : ''),
> +	    tag: 'input',
> +	    type: 'file',
> +	    size: 1,
> +	    unselectable: 'on',
> +	}, me.afterInputGuard); // Nothing special happens outside of IE/Edge
> +
> +	// This is our focusEl
> +	fileInputEl.dom.setAttribute('data-componentid', me.id);
> +
> +	if (me.tabIndex !== null) {
> +	    me.setTabIndex(me.tabIndex);
> +	}
> +
> +	if (me.accept) {
> +	    fileInputEl.dom.setAttribute('accept', me.accept);
> +	}
> +
> +	// We place focus and blur listeners on fileInputEl to activate Button's
> +	// focus and blur style treatment
> +	listeners = {
nit: Declare `listeners` here directly instead of above?

> +	    scope: me,
> +	    change: me.fireChange,
> +	    mousedown: me.handlePrompt,
> +	    keydown: me.handlePrompt,
> +	    focus: me.onFileFocus,
> +	    blur: me.onFileBlur,
> +	};
> +
> +	if (me.useTabGuards) {
> +	    listeners.keydown = me.onFileInputKeydown;
> +	}
> +
> +	fileInputEl.on(listeners);
> +    },
> +});
> diff --git a/www/manager6/lxc/CreateWizard.js b/www/manager6/lxc/CreateWizard.js
> index 0b82cc1c..7cc59299 100644
> --- a/www/manager6/lxc/CreateWizard.js
> +++ b/www/manager6/lxc/CreateWizard.js
> @@ -120,16 +120,16 @@ Ext.define('PVE.lxc.CreateWizard', {
>  		    },
>  		},
>  		{
> -		    xtype: 'proxmoxtextfield',
> +		    xtype: 'textarea',
>  		    name: 'ssh-public-keys',
>  		    value: '',
> -		    fieldLabel: gettext('SSH public key'),
> +		    fieldLabel: gettext('SSH public key(s)'),
>  		    allowBlank: true,
>  		    validator: function(value) {
>  			let pwfield = this.up().down('field[name=password]');
>  			if (value.length) {
> -			    let key = PVE.Parser.parseSSHKey(value);
> -			    if (!key) {
> +			    let keys = value.indexOf('\n') !== -1 ? value.split('\n') : [ value ];
nit: eslint complains here, should be `[value]`
also: s/let/const/

> +			    if (keys.some(key => key === '' || !PVE.Parser.parseSSHKey(key))) {
>  				return "Failed to recognize ssh key";
>  			    }
>  			    pwfield.allowBlank = true;
> @@ -159,15 +159,20 @@ Ext.define('PVE.lxc.CreateWizard', {
>  		    },
>  		},
>  		{
> -		    xtype: 'filebutton',
> +		    xtype: 'pveMultiFileButton',
>  		    name: 'file',
>  		    hidden: !window.FileReader,
>  		    text: gettext('Load SSH Key File'),
>  		    listeners: {
>  			change: function(btn, e, value) {
>  			    e = e.event;
> -			    let field = this.up().down('proxmoxtextfield[name=ssh-public-keys]');
> -			    PVE.Utils.loadSSHKeyFromFile(e.target.files[0], v => field.setValue(v));
> +			    let field = this.up().down('textarea[name=ssh-public-keys]');
> +			    for (const file of e?.target?.files ?? []) {
> +				PVE.Utils.loadSSHKeyFromFile(file, v => {
> +				    let oldValue = field.getValue();
nit: s/let/const/

> +				    field.setValue(oldValue ? `${oldValue}\n${v.trim()}` : v.trim());
> +				});
> +			    }
>  			    btn.reset();
>  			},
>  		    },
> --
> 2.30.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>

Re: [pve-devel] [PATCH manager v2] fix #4758: ui: lxc wizard: allow multiple ssh keys

[Dominik Csapak]

On 7/13/23 12:09, Christoph Heiss wrote:
> 
> On Mon, Jul 03, 2023 at 04:51:16PM +0200, Dominik Csapak wrote:
>> by converting the textfield into a textarea and validate the value
>> line wise (if there is more than one line)
>>
>> also create a 'MultiFileButton' (mostly copied from extjs) that allows
>> to select multiple files at once
>>
> 
> Some small nits inline, but otherwise:
> 
> Tested-by: Christoph Heiss <c.heiss@proxmox.com>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>> changes from v2:
>> * reduces lines of code (Thanks @Thomas)
>> * implemented multi file select
>>
>>   www/manager6/Makefile                |  1 +
>>   www/manager6/form/MultiFileButton.js | 58 ++++++++++++++++++++++++++++
>>   www/manager6/lxc/CreateWizard.js     | 19 +++++----
>>   3 files changed, 71 insertions(+), 7 deletions(-)
>>   create mode 100644 www/manager6/form/MultiFileButton.js
>>
>> [..]
>> diff --git a/www/manager6/form/MultiFileButton.js b/www/manager6/form/MultiFileButton.js
>> new file mode 100644
>> index 00000000..a73662f2
>> --- /dev/null
>> +++ b/www/manager6/form/MultiFileButton.js
>> @@ -0,0 +1,58 @@
>> +// mostly copied from ExtJS FileButton, but added 'multiple' at the relevant
>> +// places so we have a file picker where one can select multiple files
>> +Ext.define('PVE.form.MultiFileButton', {
>> +    extend: 'Ext.form.field.FileButton',
>> +    alias: 'widget.pveMultiFileButton',
>> +
>> +    afterTpl: [
>> +	'<input id="{id}-fileInputEl" data-ref="fileInputEl" class="{childElCls} {inputCls}" ',
>> +	    'type="file" size="1" name="{inputName}" unselectable="on" multiple ',
>> +	    '<tpl if="accept != null">accept="{accept}"</tpl>',
>> +	    '<tpl if="tabIndex != null">tabindex="{tabIndex}"</tpl>',
>> +	'>',
>> +    ],
>> +
>> +    createFileInput: function(isTemporary) {
>> +	var me = this,
> nit: s/var/let/
> 

this...

>> +	    fileInputEl, listeners;
>> +
>> +	fileInputEl = me.fileInputEl = me.el.createChild({
> nit: Is `me.fileInputEl` used somewhere I'm not seeing? Otherwise, could
> be eliminated and just be `let fileInputEl = ..`.
> 

..this..

>> +	    name: me.inputName || me.id,
>> +	    multiple: true,
>> +	    id: !isTemporary ? me.id + '-fileInputEl' : undefined,
>> +	    cls: me.inputCls + (me.getInherited().rtl ? ' ' + Ext.baseCSSPrefix + 'rtl' : ''),
>> +	    tag: 'input',
>> +	    type: 'file',
>> +	    size: 1,
>> +	    unselectable: 'on',
>> +	}, me.afterInputGuard); // Nothing special happens outside of IE/Edge
>> +
>> +	// This is our focusEl
>> +	fileInputEl.dom.setAttribute('data-componentid', me.id);
>> +
>> +	if (me.tabIndex !== null) {
>> +	    me.setTabIndex(me.tabIndex);
>> +	}
>> +
>> +	if (me.accept) {
>> +	    fileInputEl.dom.setAttribute('accept', me.accept);
>> +	}
>> +
>> +	// We place focus and blur listeners on fileInputEl to activate Button's
>> +	// focus and blur style treatment
>> +	listeners = {
> nit: Declare `listeners` here directly instead of above?

.. and this is imho not really relevant.
i just copied the code from the original extjs code
and adapted only a few lines (i could have emphasized this more)

for such code we normally don't change much except fix eslint warnings/errors

ofc i can improve upon it, but the intention here is to deviate
as little as possible from extjs code

> 
>> +	    scope: me,
>> +	    change: me.fireChange,
>> +	    mousedown: me.handlePrompt,
>> +	    keydown: me.handlePrompt,
>> +	    focus: me.onFileFocus,
>> +	    blur: me.onFileBlur,
>> +	};
>> +
>> +	if (me.useTabGuards) {
>> +	    listeners.keydown = me.onFileInputKeydown;
>> +	}
>> +
>> +	fileInputEl.on(listeners);
>> +    },
>> +});
>> diff --git a/www/manager6/lxc/CreateWizard.js b/www/manager6/lxc/CreateWizard.js
>> index 0b82cc1c..7cc59299 100644
>> --- a/www/manager6/lxc/CreateWizard.js
>> +++ b/www/manager6/lxc/CreateWizard.js
>> @@ -120,16 +120,16 @@ Ext.define('PVE.lxc.CreateWizard', {
>>   		    },
>>   		},
>>   		{
>> -		    xtype: 'proxmoxtextfield',
>> +		    xtype: 'textarea',
>>   		    name: 'ssh-public-keys',
>>   		    value: '',
>> -		    fieldLabel: gettext('SSH public key'),
>> +		    fieldLabel: gettext('SSH public key(s)'),
>>   		    allowBlank: true,
>>   		    validator: function(value) {
>>   			let pwfield = this.up().down('field[name=password]');
>>   			if (value.length) {
>> -			    let key = PVE.Parser.parseSSHKey(value);
>> -			    if (!key) {
>> +			    let keys = value.indexOf('\n') !== -1 ? value.split('\n') : [ value ];
> nit: eslint complains here, should be `[value]`

oops, did not see that

> also: s/let/const/
> 

hmm... we don't really have a style recommendation which to prefer.
maybe we should improve our style guideline to have some more hints when to use const
and when to use let?
(even if it's just 'use const whenever possible')

is there a strong reason here to use const vs let?
(is there in the general case, besides the immutable nature of the variable?)


>> +			    if (keys.some(key => key === '' || !PVE.Parser.parseSSHKey(key))) {
>>   				return "Failed to recognize ssh key";
>>   			    }
>>   			    pwfield.allowBlank = true;
>> @@ -159,15 +159,20 @@ Ext.define('PVE.lxc.CreateWizard', {
>>   		    },
>>   		},
>>   		{
>> -		    xtype: 'filebutton',
>> +		    xtype: 'pveMultiFileButton',
>>   		    name: 'file',
>>   		    hidden: !window.FileReader,
>>   		    text: gettext('Load SSH Key File'),
>>   		    listeners: {
>>   			change: function(btn, e, value) {
>>   			    e = e.event;
>> -			    let field = this.up().down('proxmoxtextfield[name=ssh-public-keys]');
>> -			    PVE.Utils.loadSSHKeyFromFile(e.target.files[0], v => field.setValue(v));
>> +			    let field = this.up().down('textarea[name=ssh-public-keys]');
>> +			    for (const file of e?.target?.files ?? []) {
>> +				PVE.Utils.loadSSHKeyFromFile(file, v => {
>> +				    let oldValue = field.getValue();
> nit: s/let/const/
> 

as above

>> +				    field.setValue(oldValue ? `${oldValue}\n${v.trim()}` : v.trim());
>> +				});
>> +			    }
>>   			    btn.reset();
>>   			},
>>   		    },
>> --
>> 2.30.2
>>
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>

Re: [pve-devel] [PATCH manager v2] fix #4758: ui: lxc wizard: allow multiple ssh keys

[Thomas Lamprecht]

Am 17/07/2023 um 11:03 schrieb Dominik Csapak:
> On 7/13/23 12:09, Christoph Heiss wrote:
>> also: s/let/const/
>>
> 
> hmm... we don't really have a style recommendation which to prefer.
> maybe we should improve our style guideline to have some more hints when to use const
> and when to use let?
> (even if it's just 'use const whenever possible')

Meh, I don't think `const` gives us much in a dynamic language like JS, where
one then even has to ensure that one didn't write to a const marked value (which
was only marked as such for above reason), let vs. var on the other hand gives
us clear wins and avoids bugs, so using let as default, and const if it really is
a constant seems more sensible to me.

I adapted our JS style guide a bit, should be clearer (I don't want to "forbid"
const, but using let is preferred):

https://pve.proxmox.com/wiki/Javascript_Style_Guide#Variables 

> 
> is there a strong reason here to use const vs let?
> (is there in the general case, besides the immutable nature of the variable?)

IMO no, especially as the content a variable for an object or array points
to stays modifiable, so it might be even confusing.