[pve-devel] [PATCH manager 5/6] ui: CPUModelSelector: use API call for store

[Dominik Csapak]

comments inline

On 4/22/20 3:39 PM, Stefan Reiter wrote:
> CPU models are retrieved from the new /nodes/X/cpu call and ordered by
> vendor to approximate the previous sort order (less change for accustomed
> users).
> 
> With this, custom CPU models are now selectable via the GUI.
> 
> Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
> ---
>   www/manager6/form/CPUModelSelector.js | 221 ++++++++++----------------
>   1 file changed, 81 insertions(+), 140 deletions(-)
> 
> diff --git a/www/manager6/form/CPUModelSelector.js b/www/manager6/form/CPUModelSelector.js
> index 1d28ee88..cdb7ddcb 100644
> --- a/www/manager6/form/CPUModelSelector.js
> +++ b/www/manager6/form/CPUModelSelector.js
> @@ -1,9 +1,19 @@
> +Ext.define('PVE.data.CPUModel', {
> +    extend: 'Ext.data.Model',
> +    fields: [
> +	{name: 'name'},
> +	{name: 'vendor'},
> +	{name: 'custom'},
> +	{name: 'displayname'}
> +    ]
> +});
> +
>   Ext.define('PVE.form.CPUModelSelector', {
>       extend: 'Proxmox.form.ComboGrid',
>       alias: ['widget.CPUModelSelector'],
>   
> -    valueField: 'value',
> -    displayField: 'value',
> +    valueField: 'name',
> +    displayField: 'displayname',
>   
>       emptyText: Proxmox.Utils.defaultText + ' (kvm64)',
>       allowBlank: true,
> @@ -19,157 +29,88 @@ Ext.define('PVE.form.CPUModelSelector', {
>   	columns: [
>   	    {
>   		header: gettext('Model'),
> -		dataIndex: 'value',
> +		dataIndex: 'displayname',
>   		hideable: false,
>   		sortable: true,
> -		flex: 2
> +		flex: 3
>   	    },
>   	    {
>   		header: gettext('Vendor'),
>   		dataIndex: 'vendor',
>   		hideable: false,
>   		sortable: true,
> -		flex: 1
> +		flex: 2
>   	    }
>   	],
> -	width: 320
> +	width: 360
>       },
>   
>       store: {
> -	fields: [ 'value', 'vendor' ],
> -	data: [
> -	    {
> -		value: 'athlon',
> -		vendor: 'AMD'
> -	    },
> -	    {
> -		value: 'phenom',
> -		vendor: 'AMD'
> -	    },
> -	    {
> -		value: 'Opteron_G1',
> -		vendor: 'AMD'
> -	    },
> -	    {
> -		value: 'Opteron_G2',
> -		vendor: 'AMD'
> -	    },
> -	    {
> -		value: 'Opteron_G3',
> -		vendor: 'AMD'
> -	    },
> -	    {
> -		value: 'Opteron_G4',
> -		vendor: 'AMD'
> -	    },
> -	    {
> -		value: 'Opteron_G5',
> -		vendor: 'AMD'
> -	    },
> -	    {
> -		value: 'EPYC',
> -		vendor: 'AMD'
> -	    },
> -	    {
> -		value: '486',
> -		vendor: 'Intel'
> -	    },
> -	    {
> -		value: 'core2duo',
> -		vendor: 'Intel'
> -	    },
> -	    {
> -		value: 'coreduo',
> -		vendor: 'Intel'
> -	    },
> -	    {
> -		value: 'pentium',
> -		vendor: 'Intel'
> -	    },
> -	    {
> -		value: 'pentium2',
> -		vendor: 'Intel'
> -	    },
> -	    {
> -		value: 'pentium3',
> -		vendor: 'Intel'
> -	    },
> -	    {
> -		value: 'Conroe',
> -		vendor: 'Intel'
> -	    },
> -	    {
> -		value: 'Penryn',
> -		vendor: 'Intel'
> -	    },
> -	    {
> -		value: 'Nehalem',
> -		vendor: 'Intel'
> -	    },
> -	    {
> -		value: 'Westmere',
> -		vendor: 'Intel'
> -	    },
> -	    {
> -		value: 'SandyBridge',
> -		vendor: 'Intel'
> -	    },
> -	    {
> -		value: 'IvyBridge',
> -		vendor: 'Intel'
> -	    },
> -	    {
> -		value: 'Haswell',
> -		vendor: 'Intel'
> -	    },
> -	    {
> -		value: 'Haswell-noTSX',
> -		vendor: 'Intel'
> -	    },
> -	    {
> -		value: 'Broadwell',
> -		vendor: 'Intel'
> -	    },
> -	    {
> -		value: 'Broadwell-noTSX',
> -		vendor: 'Intel'
> -	    },
> -	    {
> -		value: 'Skylake-Client',
> -		vendor: 'Intel'
> -	    },
> -	    {
> -		value: 'Skylake-Server',
> -		vendor: 'Intel'
> -	    },
> -	    {
> -		value: 'Cascadelake-Server',
> -		vendor: 'Intel'
> -	    },
> -	    {
> -		value: 'KnightsMill',
> -		vendor: 'Intel'
> -	    },
> -	    {
> -		value: 'kvm32',
> -		vendor: 'QEMU'
> -	    },
> -	    {
> -		value: 'kvm64',
> -		vendor: 'QEMU'
> -	    },
> -	    {
> -		value: 'qemu32',
> -		vendor: 'QEMU'
> -	    },
> -	    {
> -		value: 'qemu64',
> -		vendor: 'QEMU'
> -	    },
> -	    {
> -		value: 'host',
> -		vendor: 'Host'
> +	autoLoad: true,
> +	model: 'PVE.data.CPUModel',
> +	proxy: {
> +	    type: 'proxmox',
> +	    url: '/api2/json/nodes/localhost/cpu'
> +	},
> +	sorters: [
> +	    {
> +		sorterFn: function(recordA, recordB) {
> +		    let a = recordA.data;
> +		    let b = recordB.data;
> +
> +		    let vendorOrder = {
> +			"AMD": 1,
> +			"Intel": 2,
> +			"QEMU": 3,
> +			"Host": 4,
> +			"_default_": 5
> +		    };
> +
> +		    let orderA = vendorOrder[a.vendor] || vendorOrder['_default_'];
> +		    let orderB = vendorOrder[b.vendor] || vendorOrder['_default_'];
> +
> +		    if (orderA > orderB) {
> +			return 1;
> +		    } else if (orderA < orderB) {
> +			return -1;
> +		    }
> +
> +		    // Within same vendor, sort alphabetically
> +		    return a.name.localeCompare(b.name);
> +		},

the sorterFn looks correct, and altough i generally do not
try to argue about performance in javascript
i would imagine that having the order object outside
instead of declaring it everytime the function is called
would be faster.. (i am not sure how smart the jit actually is
in that case...)
so having e.g. PVE.Utils.CPUVendorOrder would be ok

also, you could add a 'calculated' field  in the model
which just concatenates the orderindex with the
name and let it string sort by it

e.g. (above with the fields)

{
name: 'sortname',
calculated: function(data) {
	let order = PVE.Utils.CPUVendorOrder[data.vendor] || ....;
	return `${order}${data.name}`;
}
}

and then sort by this field

if you go by this route, i would suggest using characters for the order 
instead of numbers (since that scales only until 10, then
you'd have to prefix them with 0 somewhere...)
so
AMD: 'a'
Intel: 'b'
QEMU: 'c'
etc.

> +		direction: 'ASC'
> +	    }
> +	],
> +	listeners: {
> +	    load: function(store, records, success) {
> +		if (success) {
> +		    records.forEach(rec => {
> +			rec.data.displayname = rec.data.name.replace(/^custom-/, '');
> +
> +			let vendor = rec.data.vendor;
> +
> +			if (rec.data.name === 'host') {
> +			    vendor = 'Host';
> +			}
> +
> +			// We receive vendor names as given to QEMU as CPUID
> +			let vendorMap = {
> +			    'default': 'QEMU',
> +			    'AuthenticAMD': 'AMD',
> +			    'GenuineIntel': 'Intel'
> +			};

same comment about the declaration here, i'd rather put this in 
PVE.Utils (or in this class as static member)

> +			vendor = vendorMap[vendor] || vendor;
> +
> +			if (rec.data.custom) {
> +			    vendor = `Custom (${vendor})`;

gettext for custom is missing here

> +			}
> +
> +			rec.data.vendor = vendor;
> +		    });
> +
> +		    store.sort();
> +		}
>   	    }
> -	]
> +	}
>       }
>   });
>