[pve-devel] [PATCH v3 widget-toolkit] add network selector widget

[Dominik Csapak]

high level looks ok, some comments inline

On 6/11/19 2:04 PM, Tim Marx wrote:
> Signed-off-by: Tim Marx <t.marx at proxmox.com>
> ---
>   Makefile                |   1 +
>   form/NetworkSelector.js | 121 ++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 122 insertions(+)
>   create mode 100644 form/NetworkSelector.js
> 
> diff --git a/Makefile b/Makefile
> index b9dc8b9..d12a4da 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -34,6 +34,7 @@ JSSRC=					\
>   	form/ComboGrid.js		\
>   	form/RRDTypeSelector.js		\
>   	form/BondModeSelector.js	\
> +	form/NetworkSelector.js		\
>   	button/Button.js		\
>   	button/HelpButton.js		\
>   	grid/ObjectGrid.js		\
> diff --git a/form/NetworkSelector.js b/form/NetworkSelector.js
> new file mode 100644
> index 0000000..fa512e7
> --- /dev/null
> +++ b/form/NetworkSelector.js
> @@ -0,0 +1,121 @@
> +Ext.define('Proxmox.form.NetworkSelectorController', {
> +    extend: 'Ext.app.ViewController',
> +    alias: 'controller.proxmoxNetworkSelectorController',
> +
> +    init: function(view) {
> +	var me = this;
> +
> +	if (!view.nodename) {
> +	    throw "missing custom view config: nodename";
> +	}
> +	view.getStore().getProxy().setUrl('/api2/json/nodes/'+ view.nodename + '/network');
> +    }
> +});

any reason why you define it outside of the NetworkSelector (like we do 
everywhere else with xclass: 'Ext.app.ViewController' ?)

> +
> +Ext.define('Proxmox.data.NetworkSelector', {
> +    extend: 'Ext.data.Model',
> +    fields: [
> +	{name: 'active'},
> +	{name: 'cidr'},
> +	{name: 'cidr6'},
> +	{name: 'comments'},
> +	{name: 'iface'},
> +	{name: 'slaves'},
> +	{name: 'type'}
> +    ]
> +});
> +
> +Ext.define('Proxmox.form.NetworkSelector', {
> +    extend: 'Proxmox.form.ComboGrid',
> +    alias: 'widget.proxmoxNetworkSelector',
> +
> +    nodename: 'localhost',
> +    controller: 'proxmoxNetworkSelectorController',
> +    setNodename: function(nodename) {
> +	this.nodename = nodename;
> +	var networkSelectorStore = this.getStore();
> +	networkSelectorStore.removeAll();
> +	// because of manual local copy of data for ip4/6
> +	this.getPicker().refresh();
> +	if (networkSelectorStore && typeof networkSelectorStore.getProxy === 'function') {
> +	    networkSelectorStore.getProxy().setUrl('/api2/json/nodes/'+ nodename + '/network');
> +	    networkSelectorStore.load();
> +	}
> +    },
> +    // set default value to empty array, else it inits it with
> +    // null and after the store load it is an empty array,
> +    // triggering dirtychange
> +    value: [],
> +    valueField: 'cidr',
> +    displayField: 'cidr',
> +    store: {
> +	autoLoad: true,
> +	model: 'Proxmox.data.NetworkSelector',
> +	proxy: {
> +	    type: 'proxmox'
> +	},
> +	sorters: [
> +	    {
> +		property : 'iface',
> +		direction: 'ASC'
> +	    }
> +	],
> +	filters: [
> +	      function(item) {
> +		return item.data.cidr;
we should not ignore interfaces with ipv6 only

> +	    }
> +	],
> +	listeners: {
> +	    load: function(records, successfull) {

this signature is wrong, the load event has function(store, records, 
success,...)

it works by accident since the this is the store, the store has an 
'each' method  if there are no records it does not call your function

> +		var store = this;
> +		if(successfull) {
> +		    records.each(function(record) {
> +			if(record.data.cidr && record.data.cidr6) {
> +			    var tempcopy = record.copy(null);
> +			    tempcopy.data.cidr = tempcopy.data.cidr6;
> +			    tempcopy.data.comment = tempcopy.data.comments6;
> +			    store.add(tempcopy);
> +			}
> +		    });
> +		}
> +	    }
> +	}
> +    },
> +    listConfig: {
> +	width: 600,
> +	columns: [
> +	    {
> +		header: gettext('Interface'),
> +		sortable: true,
> +		flex:1,
> +		dataIndex: 'iface'
> +	    },
> +	    {
> +		header: gettext('Active'),
> +		sortable: true,
> +		flex:1,
> +		dataIndex: 'active'
> +	    },
> +	    {
> +
> +		header: gettext('CIDR'),
> +		dataIndex: 'cidr',
> +		sortable: true,
> +		hideable: false,
> +		flex:1
> +	    },

same here regarding ignoring ipv6 only, if we have an interface with 
ipv6 only, it does not show up better would be probably a 'calculated 
field' that returns
cidr || cidr6
and show that

> +	    {
> +		header: gettext('Type'),
> +		sortable: true,
> +		flex:1,
> +		dataIndex: 'type'
> +	    },
> +	    {
> +		header: gettext('Comment'),
> +		sortable: true,
> +		flex:1,
> +		dataIndex: 'comments'
> +	    }
> +	]
> +    }
> +});
>