Re: [PATCH v2 1/2] perf hists browser: Support horizontal scrolling with '<' and '>' key

[Namhyung Kim <namhyung@kernel.org>]

Hi Arnaldo,

On Tue, Aug 11, 2015 at 05:59:28PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Aug 11, 2015 at 01:15:59AM +0200, Jiri Olsa escreveu:
> > On Mon, Aug 10, 2015 at 08:14:45PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Tue, Aug 11, 2015 at 01:02:56AM +0200, Jiri Olsa escreveu:
> > > > On Mon, Aug 10, 2015 at 07:58:22PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > > > it'd really help for perf mem output, which is quite wide.. and we probably
> > > > > > have more wide outputs, or users with narrow terminals ;-)
> 
> > > > > I'm not going against horizontal scrolling, it is needed, sure thing,
> > > > > its surprising we are doing this only now. What I am asking is this fine
> > > > > scrolling of one column per <- or -> keypress, but I really need to try
> > > > > it with things like 'perf mem', I thought that when you pressed '>', in
> > > > > this patch, it would move entire sort key columns, not just one vertical
> > > > > column one character wide, right?
> 
> > > > yep, the whole sort column seemed more usefull,
> 
> So, that wasn't what was implemented in Namhyung's patchkit, right? I.e.
> it scrolls characters, not columns.

My last patch already implemented the scrolling by columns as well as
characters.


> 
> Can you take a look at the patchkit I put together at my tree, branch:
> 
> 	tmp.perf/ui_browser.horiz_scroll
> 
> https://git.kernel.org/cgit/linux/kernel/git/acme/linux.git/log/?h=tmp.perf/ui_browser.horiz_scroll
> 
> The last two patches do it:
> 
>   $ git log --oneline | head -2

You can simply use 'git log --oneline -2'. :)


>   8a7684de198b perf hists browser: Implement horizontal scrolling
>   a00e506da09e perf ui browser: Optional horizontal scrolling key binding
>   $
> 
>   $ git diff HEAD^^ --stat
>    tools/perf/ui/browser.c        | 14 ++++++++++++++
>    tools/perf/ui/browser.h        |  2 +-
>    tools/perf/ui/browsers/hists.c | 22 +++++++++++++++++-----
>    3 files changed, 32 insertions(+), 6 deletions(-)
>   $

Looks good.  It's much simpler than mine and I think it's good if we
decided to support only column scrolling.


> 
> Several lines are just comments explaining some tricks due to me not
> having found a counter for the number of colums somewhere and reusing
> the first loop that traverses them all to do the counting.
> 
> There are two before those that at first I thought was needed, but ended
> up not using (would have to render the whole line in ui_browser to do
> the scrolling at line printing time, works only for browsers where just
> one call to ui_browser__printf or ui_browser__write_nstring is done,
> but I ended up leaving it there anyway, to try to make the
> hist_browser.c and other ui_browser implementations (annotate, etc)
> independent of libslang:
> 
>   $ git log --oneline | head -4 | tail -2
>   e7534e88dfa3 perf ui browser: Introduce ui_browser__printf()
>   2fe0f7e4b73e perf ui browser: Introduce ui_browser__write_nstring()
>   $
> 
> Tested it with 'perf mem record -g -a' + 'perf mem report', and I liked
> how it works, please check if you like it too :-)
> 
> The <- and -> keys are reused just when the horizontal scrolling mode is
> activate by setting ui_browser->columns, the hists_browser (perf report,
> perf top) will continue having ENTER and ESC, as always, to
> select/deselect things.

Currently the help message in the hist browser says the arrows keys
are used to zoom in & out and ESC is for 'exit browser'.  Do you think
it's ok to change the current behavior?


> 
> If we insist we need character by character scrolling, or if we need
> that move in other browser (annotate, for instance) its just a matter of
> using ui_browser->horiz_scroll as a char counter and use it in the
> ui_browser->refresh() calls when rendering each line.

Yes, I think it's needed and it'd be great if other browsers support it.

Thanks,
Namhyung