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

[Dominik Csapak <d.csapak@proxmox.com>]

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