Message-ID: <20150812131753.GC31059@kernel.org>
Date:	Wed, 12 Aug 2015 10:17:53 -0300
From:	Arnaldo Carvalho de Melo <acme@...nel.org>
To:	Namhyung Kim <namhyung@...nel.org>
Cc:	Jiri Olsa <jolsa@...hat.com>, Ingo Molnar <mingo@...nel.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	LKML <linux-kernel@...r.kernel.org>,
	David Ahern <dsahern@...il.com>,
	Andi Kleen <andi@...stfloor.org>
Subject: Re: [PATCH v2 1/2] perf hists browser: Support horizontal scrolling
 with '<' and '>' key

Em Wed, Aug 12, 2015 at 02:41:22PM +0900, Namhyung Kim escreveu:
> 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.

Oh, didn't notice, should have read it more carefully :-\
 
> > 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'. :)

thanks :)
 
> >   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.

Even if we decide to support both, which I agree, this way of
implementing horizontal scrolling, as you said, is really simple, and
all browsers wanting horizontal scrolling will want these basic blocks,
see below for char by char horizontal 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?

I think it is.
 
> > 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.

So, with ui_browser__write_nstring(), ui_browser__printf() wrapped,
we're left with the other methods dealing with graph characters to
support char-by-char working for horizontal scrolling.

What I think we should do is to buffer all the updates to the screen,
then, right after returning from ui_browser->refresh(), traverse the
list of formatted lines and chop off ui_browser->horiz_scroll from the
start of each line.

But this requires a bit more work, to think about gotorc, etc, which I
think can be left for later, as column scrolling in the simple way done
now seems to serve us well for things like 'perf mem report', which I
think is _the_ use case now, right?

What I don't want to see is us overcomplicating browser by browser to do
things that all of them need to do :-\

With regard to using Alt+-> for column by column and -> for
char by char horizontal scrolling, I think we can have instead a hotkey
we press to toggle the scrolling mode, i.e. we start with either way,
the one considered most useful (column by column IMNSHO) and if we
press, say, '.', the we switch to using the other one (char by char),
the behaviour could be selected by ~/.perfconfig setting too, of course.

- Arnald
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists