Re: [PATCH v2 5/5] perf report: Initial TUI using newt

[Ingo Molnar <mingo@elte.hu>]

* Arnaldo Carvalho de Melo <acme@infradead.org> wrote:

> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> Newt has widespread availability and provides a rather simple API as can be 
> seen by the size of this patch.
> 
> The work needed to support it will benefit other frontends too.
> 
> In this initial patch it just checks if the output is a tty, if not it falls 
> back to the previous behaviour, also if newt-devel/libnewt-dev is not 
> installed the previous behaviour is maintaned.
> 
> Pressing enter on a symbol will annotate it, ESC in the annotation window will
> return to the report symbol list.

Very nice!

a few observations. Firstly, could we perhaps make most of the interface 
functions GUI/TUI invariant? I.e. things like:

> +	if (use_browser)
> +		r = vfprintf(fp, fmt, args);
> +	else
> +		r = color_vfprintf(fp, color, fmt, args);

should be abstracted away into a single method:

	r = color_vprintf(fp, color, fmt, args);

where color_vprintf() knows about which current GUI front-end to use.

(The old color_printf() should be renamed to ascii_color_printf() or so, and 
put into a front-end driver structure perhaps - instead of explicit flags.)

There's a similar situation here too:

> -		ret = vfprintf(stderr, fmt, args);
> +		if (use_browser)
> +			ret = browser__show_help(fmt, args);
> +		else
> +			ret = vfprintf(stderr, fmt, args);

Plus a few other observations about the newt TUI itself:

 - The most important first-impression thing in a TUI is to make it obvious to
   exit it. I eventually found that Escape would exit - but it would be nice 
   to map 'Q' and 'Ctrl-C' to it as well. Nothing is more annoying than a TUI 
   you cannot exit from.

 - There's still a 'perf annotate' bug that has been introduced recently, and 
   it shows up in the TUI too. The bug is due to us passing this to objdump 
   and grep:

     18573 execve("/bin/sh", ["sh", "-c", "objdump --start-address=0xffffffff81387b36
       --stop-address=0xffffffff81387b4f -dS [kernel.kallsyms]|grep -v [kernel.kallsyms]"]

   Look at how [kernel.kallsyms] goes unquoted to the shell, so globbing will 
   match it on random file names in the current directory - which will then be 
   showed by objdump, much to the surprise of the user!

 - I suspect we should finally make use of the .perfconfig parser and enable people
   to use a different front-end from Newt? Just in case they prefer ASCII.

 - When i hit enter on a symbol to annotate it, but the annotation fails, the TUI
   just does nothing currently. Instead it should print something informative 
   (and eye-catching) into a status line at the top or the bottom of the 
   screen, possibly printed in red characters or so. Not a separate window as that
   needs extra key-hits to get rid of - just a sufficiently visible status 
   line would be perfect. There can be a few reasons why some functions can be 
   annotated while others cannot be.

 - [ call-graph data is not represented yet :-) ]

Anyway, very nice stuff!

	Ingo