Re: [PATCH pve-manager v4 12/17] ui: add basic custom CPU model editor

[Fiona Ebner <f.ebner@proxmox.com>]

Am 30.04.26 um 6:00 PM schrieb Arthur Bied-Charreton:
> diff --git a/www/manager6/dc/CPUTypeEdit.js b/www/manager6/dc/CPUTypeEdit.js
> new file mode 100644
> index 00000000..594772ed
> --- /dev/null
> +++ b/www/manager6/dc/CPUTypeEdit.js
> +    items: [
> +        {
> +            xtype: 'inputpanel',
> +            onGetValues: function (values) {
> +                let win = this.up('window');
> +
> +                PVE.Utils.delete_if_default(values, 'reported-model', '', win.isCreate);
> +                PVE.Utils.delete_if_default(values, 'hv-vendor-id', '', win.isCreate);
> +                PVE.Utils.delete_if_default(values, 'phys-bits', '', win.isCreate);
> +                PVE.Utils.delete_if_default(values, 'flags', '', win.isCreate);

Should we also do this for the 'hidden' property?

> +
> +                if (win.isCreate) {
> +                    delete values.delete;
> +                }
> +
> +                return values;
> +            },
> +            column1: [
> +                {
> +                    xtype: 'pmxDisplayEditField',
> +                    fieldLabel: gettext('Name'),
> +                    cbind: {
> +                        editable: '{isCreate}',
> +                        value: '{cputype}',
> +                    },
> +                    name: 'cputype',
> +                    renderer: (val) => val.replace(/^custom-/, ''),
> +                    allowBlank: false,
> +                },
> +                {
> +                    xtype: 'CPUModelSelector',
> +                    fieldLabel: gettext('Base Model'),
> +                    showCustomModels: false,
> +                    name: 'reported-model',
> +                    autoEl: {
> +                        tag: 'div',
> +                        'data-qtip': gettext(
> +                            'CPU model the rest of the configuration will be based on.',

Nit: s/will be/is/
This in particular makes it sound slightly more correct when editing and
we can still use the same gettext for the grid below.

Since the historic (back-end) default is the non-recommended kvm64
model, I think we should have the user make an actual choice here and
disallow empty for the model selector field. At least in the isCreate
case, otherwise I guess we need to keep it working for compat with
pre-existing manually created custom models.

> diff --git a/www/manager6/dc/CPUTypeView.js b/www/manager6/dc/CPUTypeView.js
> new file mode 100644
> index 00000000..207673a7
> --- /dev/null
> +++ b/www/manager6/dc/CPUTypeView.js
> +        {
> +            header: gettext('Base Model'),
> +            flex: 1,
> +            dataIndex: 'reported-model',
> +            autoEl: {
> +                tag: 'div',
> +                'data-qtip': gettext('CPU model the rest of the configuration will be based on.'),

Nit: s/will be/is/

> +            },
> +        },
> +        {
> +            header: gettext('Phys-Bits'),
> +            flex: 1,
> +            dataIndex: 'phys-bits',
> +        },
> +        {
> +            header: gettext('Hidden'),
> +            flex: 1,
> +            dataIndex: 'hidden',
> +        },
> +        {
> +            header: gettext('HyperV-Vendor'),
> +            flex: 1,
> +            dataIndex: 'hv-vendor-id',
> +        },
> +        {
> +            header: gettext('Flags'),
> +            flex: 2,
> +            dataIndex: 'flags',
> +        },
> +    ],
> +
> +    tbar: [
> +        {
> +            text: gettext('Add'),
> +            handler: 'onAdd',
> +        },
> +        '-',
> +        {
> +            xtype: 'proxmoxStdRemoveButton',
> +            baseurl: '/api2/extjs/cluster/qemu/custom-cpu-models/',
> +            getRecordName: (rec) => rec.data.cputype,
> +            getUrl: function (rec) {
> +                let me = this;
> +                return me.baseurl + rec.data.cputype;
> +            },
> +            confirmMsg: function (rec) {
> +                return Ext.String.format(
> +                    gettext('Are you sure you want to remove the custom CPU model {0}'),
> +                    `"${rec.data.cputype.replace(/^custom-/, '')}"?`,

The question mark should be part of the template string

> diff --git a/www/manager6/form/PhysBitsSelector.js b/www/manager6/form/PhysBitsSelector.js
> new file mode 100644
> index 00000000..78f932a6
> --- /dev/null
> +++ b/www/manager6/form/PhysBitsSelector.js
> +    items: [
> +        {
> +            xtype: 'radiofield',
> +            boxLabel: gettext('Default'),
> +            inputValue: 'default',
> +            checked: true,
> +            reference: 'modeDefault',
> +            listeners: {
> +                change: 'onRadioChange',
> +            },
> +            isFormField: false,
> +        },
> +        {
> +            xtype: 'radiofield',
> +            boxLabel: gettext('Host'),
> +            inputValue: 'host',
> +            reference: 'modeHost',
> +            listeners: {
> +                change: 'onRadioChange',
> +            },
> +            isFormField: false,
> +        },
> +        {
> +            xtype: 'fieldcontainer',
> +            layout: 'hbox',
> +            items: [
> +                {
> +                    xtype: 'radiofield',
> +                    boxLabel: gettext('Custom'),
> +                    inputValue: 'custom',
> +                    listeners: {
> +                        change: 'onRadioChange',
> +                    },
> +                    reference: 'modeCustom',
> +                    isFormField: false,
> +                },

I feel like we could be a bit more descriptive here, maybe:
Default from QEMU
Inherit from host CPU
Custom number
?

> +                {
> +                    xtype: 'numberfield',
> +                    width: 60,
> +                    margin: '0 0 0 10px',
> +                    minValue: 8,
> +                    maxValue: 64,
> +                    reference: 'customNum',
> +                    allowBlank: false,
> +                    isFormField: false,
> +                    disabled: true,
> +                },
> +            ],
> +        },
> +    ],
> +});