Re: [pve-devel] [PATCH v2 manager] gui: expose content-dirs property in storage edit/create

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 14/03/2023 um 14:14 schrieb Leo Nunner:
> Add a separate tab for the storage edit/create panels to set the
> recently introduced "content-dirs" property which overrides the
> default directory locations. Analogous to the API implementation,
> the tab was added for Directory, CIFS and NFS storages.
> 
> Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
> ---
>  www/manager6/storage/CIFSEdit.js |  8 +++
>  www/manager6/storage/DirEdit.js  | 85 ++++++++++++++++++++++++++++++++
>  www/manager6/storage/NFSEdit.js  |  8 +++
>  3 files changed, 101 insertions(+)
> 

> +Ext.define('PVE.panel.ContentDirsPanel', {
> +    extend: 'Proxmox.panel.InputPanel',

this shouldn't be squished into DirEdit, but rather live in its own file - e.g. in
panel/

> +    xtype: 'pveContentDirsTab',
> +
> +    onGetValues: function(form) {

while form isn't wrong, it has IMO merits to keep the more widely used
`values` as parameter name here.

> +	let str = PVE.Parser.printPropertyString(form);
> +	let values = { "content-dirs": str };
> +	return values;

could be as short as:

onGetValues: values => ({ "content-dirs", PVE.Parser.printPropertyString(values) }),

> +    },
> +
> +    onSetValues: function(values) {
> +	return values?.["content-dirs"];
> +    },

could be:

onSetValues: values => values?.["content-dirs"],

without loosing really any readability

> +
> +    initComponent: function() {
> +	let me = this;
> +
> +	me.column1 = [
> +	    {
> +		xtype: 'textfield',
> +		name: 'images',
> +		fieldLabel: gettext('Disk image'),

We normally use Title Case for field labels.

> +		allowBlank: true,
> +		emptyText: "images",

It would UX do good if you define a vtype (see Toolkit.js in manager or widget toolkit)
for the regex used by the backend to help users catching bogus entries early.

Above two (casing & vtype) applies for the remaining fields here.


Note also that having things like disks or the like editable for existing storages
can break things fast. I'd maybe be open for having snippets, ISOs and CT templates
editable, and even then show a warning hint that existing guest might be affected by
the change (i.e., not start anymore). FYI: pmxDisplayEditField is designed for such
mixed create/edit -> editable/display-only use-cases.