[pve-devel] [PATCH manager 3/4] gui: content view: Add checkbox for recursive search

[Dominik Csapak]

comments inline

On 4/2/20 1:34 PM, Dominic Jäger wrote:
> Default is no recursion.
> This commit depends on "Recursive search for iso and vztmpl" in pve-storage.
> 
> Signed-off-by: Dominic Jäger <d.jaeger at proxmox.com>
> ---
> Changes since RFC:
> * [0] became obsolte
> * This did not exist in RFC
> 
> [0] https://pve.proxmox.com/pipermail/pve-devel/2019-December/040886.html
> 
>   www/manager6/storage/ContentView.js | 23 +++++++++++++++++++++--
>   1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/www/manager6/storage/ContentView.js b/www/manager6/storage/ContentView.js
> index 001efc7f..5c6f1418 100644
> --- a/www/manager6/storage/ContentView.js
> +++ b/www/manager6/storage/ContentView.js
> @@ -379,12 +379,15 @@ Ext.define('PVE.storage.ContentView', {
>   	}
>   
>   	var baseurl = "/nodes/" + nodename + "/storage/" + storage + "/content";
> +	me.sp = Ext.state.Manager.getProvider();
> +
>   	var store = Ext.create('Ext.data.Store',{
>   	    model: 'pve-storage-content',
>   	    groupField: 'content',
>   	    proxy: {
>                   type: 'proxmox',
> -		url: '/api2/json' + baseurl
> +		url: '/api2/json' + baseurl + '?recursive='
> +		    + (me.sp.get('recursive-search')|0), // API expects integer

four things here:

1. i would prefer to get that info out early and simply use a variable
    let recursive = me.sp.get... above and here only use the variable
2. we now can use modern js syntax, so a template string is allowed:
    `/api2/json${baseurl}?recursive=${recursive}`
    (we could even use extjs' query string builder for this)
3. the api expects integer, but why do here convert a string
    to int to string again? why not save a string directly?
4. there is nothing in this patch that sets this
    why not introduce this in the next patch?

>   	    },
>   	    sorters: {
>   		property: 'volid',
> @@ -578,7 +581,23 @@ Ext.define('PVE.storage.ContentView', {
>   			    ]);
>   			}
>   		    }
> -		}
> +		},
> +		{
> +		    xtype: 'proxmoxcheckbox',
> +		    fieldLabel: gettext('Recursive'),
> +		    labelWidth: 65,
> +		    name : 'recursive',
> +		    checked: false,
> +		    listeners: {
> +			change: function(box, value) {
> +			    me.store.proxy.url = me.store.proxy.url.replace(
> +				/recursive=\d/,
> +				"recursive=" + (value|0) // API expects integer

i would rather have a helper function that assembles the url
and reuse it here instead of string replacement

also you can set the checked/unchecked value to strings
---
uncheckedValue: '0',
checkedValue: '0',
---
(not sure about the names, please see the docs)
then its not needed to convert to int/string

> +			    );
> +			    reload();
> +			},
> +		    },
> +		},
>   	    ],
>   	    columns: [
>   		{
>