Re: [PATCH v7 2/3] perf report: Support a new key to reload the browser

[Arnaldo Carvalho de Melo]

Em Thu, Feb 20, 2020 at 09:36:15AM +0800, Jin Yao escreveu:
> Sometimes we may need to reload the browser to update the output since
> some options are changed.
> 
> This patch creates a new key K_RELOAD. Once the __cmd_report() returns
> K_RELOAD, it would repeat the whole process, such as, read samples from
> data file, sort the data and display in the browser.
> 
>  v7:
>  ---
>  Rebase to perf/core, no other change.
> 
>  v6:
>  ---
>  No change.
> 
>  v5:
>  ---
>  1. Fix the 'make NO_SLANG=1' error. Define K_RELOAD in util/hist.h.
>  2. Skip setup_sorting() in repeat path if last key is K_RELOAD.
> 
>  v4:
>  ---
>  Need to quit in perf_evsel_menu__run if key is K_RELOAD.
> 
>  v3:
>  ---
>  No change.
> 
>  v2:
>  ---
>  This is a new patch created in v2.
> 
> Signed-off-by: Jin Yao <yao.jin@xxxxxxxxxxxxxxx>
> ---
>  tools/perf/builtin-report.c    | 6 +++---
>  tools/perf/ui/browsers/hists.c | 1 +
>  tools/perf/ui/keysyms.h        | 1 +
>  tools/perf/util/hist.h         | 1 +
>  4 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 862c7f8853dc..842ef92c3598 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -635,7 +635,7 @@ static int report__browse_hists(struct report *rep)
>  		 * Usually "ret" is the last pressed key, and we only
>  		 * care if the key notifies us to switch data file.
>  		 */
> -		if (ret != K_SWITCH_INPUT_DATA)
> +		if (ret != K_SWITCH_INPUT_DATA && ret != K_RELOAD)
>  			ret = 0;
>  		break;
>  	case 2:
> @@ -1469,7 +1469,7 @@ int cmd_report(int argc, const char **argv)
>  		sort_order = sort_tmp;
>  	}
>  
> -	if ((last_key != K_SWITCH_INPUT_DATA) &&
> +	if ((last_key != K_SWITCH_INPUT_DATA && last_key != K_RELOAD) &&
>  	    (setup_sorting(session->evlist) < 0)) {
>  		if (sort_order)
>  			parse_options_usage(report_usage, options, "s", 1);
> @@ -1548,7 +1548,7 @@ int cmd_report(int argc, const char **argv)
>  	sort__setup_elide(stdout);
>  
>  	ret = __cmd_report(&report);
> -	if (ret == K_SWITCH_INPUT_DATA) {
> +	if (ret == K_SWITCH_INPUT_DATA || ret == K_RELOAD) {
>  		perf_session__delete(session);
>  		last_key = K_SWITCH_INPUT_DATA;

Are you sure this shouldn't be:
	
		last_key = ret;

?

I'm applying it to test now anyway,

- Arnaldo

>  		goto repeat;
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index f36dee499320..7c091fa51a5c 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -3440,6 +3440,7 @@ static int perf_evsel_menu__run(struct evsel_menu *menu,
>  					pos = perf_evsel__prev(pos);
>  				goto browse_hists;
>  			case K_SWITCH_INPUT_DATA:
> +			case K_RELOAD:
>  			case 'q':
>  			case CTRL('c'):
>  				goto out;
> diff --git a/tools/perf/ui/keysyms.h b/tools/perf/ui/keysyms.h
> index fbfac29077f2..04cc4e5c031f 100644
> --- a/tools/perf/ui/keysyms.h
> +++ b/tools/perf/ui/keysyms.h
> @@ -25,5 +25,6 @@
>  #define K_ERROR	 -2
>  #define K_RESIZE -3
>  #define K_SWITCH_INPUT_DATA -4
> +#define K_RELOAD -5
>  
>  #endif /* _PERF_KEYSYMS_H_ */
> diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
> index 0aa63aeb58ec..bb994e030495 100644
> --- a/tools/perf/util/hist.h
> +++ b/tools/perf/util/hist.h
> @@ -536,6 +536,7 @@ static inline int block_hists_tui_browse(struct block_hist *bh __maybe_unused,
>  #define K_LEFT  -1000
>  #define K_RIGHT -2000
>  #define K_SWITCH_INPUT_DATA -3000
> +#define K_RELOAD -4000
>  #endif
>  
>  unsigned int hists__sort_list_width(struct hists *hists);
> -- 
> 2.17.1
> 

-- 

- Arnaldo