[pve-devel] [PATCH v3 manager] Fix #1831: Add filter to CT template/appliances download window

[Dominik Csapak]

a few comments inline

On 1/29/19 3:35 PM, Christian Ebner wrote:
> This adds the posibility to filter CT template/appliances by package as well as
> description in the CT template/appliances download window.
> 
> Signed-off-by: Christian Ebner <c.ebner at proxmox.com>
> ---
> 
> Version 3:
>      * In lack of better alternatives solved via toLowerCase() and indexOf()
>        instead of regex
> 
>   www/manager6/storage/ContentView.js | 26 ++++++++++++++++++++++++--
>   1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/www/manager6/storage/ContentView.js b/www/manager6/storage/ContentView.js
> index 6b73f437..b421287c 100644
> --- a/www/manager6/storage/ContentView.js
> +++ b/www/manager6/storage/ContentView.js
> @@ -35,11 +35,33 @@ Ext.define('PVE.grid.TemplateSelector', {
>   	    store.load();
>   	};
>   
> +	var storeFilter = function(rec) {
> +	    return (rec.data['package'].toLowerCase().indexOf(me.fval) != -1)
> +		|| (rec.data.headline.toLowerCase().indexOf(me.fval) != -1);
> +	};

please try to use === and !=== instead of == and !=
it is not strictly necessary here, but its good practice

also please remove the trailing whitespace here

> +
>   	Proxmox.Utils.monStoreErrors(me, store);
>   
>   	Ext.apply(me, {
>   	    store: store,
>   	    selModel: sm,
> +	    tbar: [
> +		'->',
> +		gettext('Filter: '),
> +		{
> +		    xtype: 'textfield',
> +		    width: 200,
> +		    enableKeyEvents: true,
> +		    listeners: {
> +			buffer: 500,
> +			keyup: function(field) {
> +			    me.fval = field.getValue().toLowerCase();
> +			    store.clearFilter(true);
> +			    store.filterBy(storeFilter);

instead of saving the value (fval is not a good name btw) in the object,
i would simply create the function inline here
eg.

var val = field.getValue().to...
store.clear...
store.filter(function(rec) {

...

});

this way the whole logic is together and one does not have to jump
around to see what the code is doing

> +			}
> +		    }
> +		}
> +	    ],
>   	    features: [ groupingFeature ],
>   	    columns: [
>   		{
> @@ -92,8 +114,8 @@ Ext.define('PVE.storage.TemplateDownload', {
>       modal: true,
>       title: gettext('Templates'),
>       layout: 'fit',
> -    width: 600,
> -    height: 400,
> +    width: 900,
> +    height: 600,
>       initComponent : function() {
>   	/*jslint confusion: true */
>           var me = this;
>