diff mbox series

[3/3] perf annotate browser: Add '<' and '>' keys for navigation

Message ID 20230511062725.514752-3-namhyung@kernel.org
State New
Headers
Series [1/3] perf annotate: Handle more instructions on x86 |

Commit Message

Namhyung Kim May 11, 2023, 6:27 a.m. UTC 
  The hists__find_annotations() allow to move to next or previous symbols
for annotation using the arrow keys.  But TUI annotate_browser__run()
uses the RIGHT key as ENTER to handle jump/call instructions.  That
makes the navigation to the next function impossible.

I'd like to change it back to move the next symbol but I'm afraid if
some users get confused.  So I added a new pair of keys to handle that.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-annotate.c     | 4 +++-
 tools/perf/ui/browsers/annotate.c | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

Comments

Ian Rogers May 15, 2023, 5:09 p.m. UTC | #1 
On Wed, May 10, 2023 at 11:27 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> The hists__find_annotations() allow to move to next or previous symbols
> for annotation using the arrow keys.  But TUI annotate_browser__run()
> uses the RIGHT key as ENTER to handle jump/call instructions.  That
> makes the navigation to the next function impossible.
>
> I'd like to change it back to move the next symbol but I'm afraid if
> some users get confused.  So I added a new pair of keys to handle that.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/builtin-annotate.c     | 4 +++-
>  tools/perf/ui/browsers/annotate.c | 4 +++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> index 63cdf6ea6f6d..425a7e2fd6fb 100644
> --- a/tools/perf/builtin-annotate.c
> +++ b/tools/perf/builtin-annotate.c
> @@ -342,7 +342,7 @@ static void hists__find_annotations(struct hists *hists,
>                 notes = symbol__annotation(he->ms.sym);
>                 if (notes->src == NULL) {
>  find_next:
> -                       if (key == K_LEFT)
> +                       if (key == K_LEFT || key == '<')
>                                 nd = rb_prev(nd);
>                         else
>                                 nd = rb_next(nd);
> @@ -378,9 +378,11 @@ static void hists__find_annotations(struct hists *hists,
>                                         return;
>                                 /* fall through */
>                         case K_RIGHT:
> +                       case '>':
>                                 next = rb_next(nd);
>                                 break;
>                         case K_LEFT:
> +                       case '<':
>                                 next = rb_prev(nd);
>                                 break;
>                         default:
> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> index 12c3ce530e42..70bad42b807b 100644
> --- a/tools/perf/ui/browsers/annotate.c
> +++ b/tools/perf/ui/browsers/annotate.c
> @@ -781,9 +781,9 @@ static int annotate_browser__run(struct annotate_browser *browser,
>                         ui_browser__help_window(&browser->b,
>                 "UP/DOWN/PGUP\n"
>                 "PGDN/SPACE    Navigate\n"
> +               "</>           Move to prev/next symbol\n"
>                 "q/ESC/CTRL+C  Exit\n\n"
>                 "ENTER         Go to target\n"
> -               "ESC           Exit\n"
>                 "H             Go to hottest instruction\n"
>                 "TAB/shift+TAB Cycle thru hottest instructions\n"
>                 "j             Toggle showing jump to target arrows\n"
> @@ -913,6 +913,8 @@ static int annotate_browser__run(struct annotate_browser *browser,
>                         annotation__toggle_full_addr(notes, ms);
>                         continue;
>                 case K_LEFT:
> +               case '<':
> +               case '>':
>                 case K_ESC:
>                 case 'q':
>                 case CTRL('c'):
> --
> 2.40.1.521.gf1e218fcd8-goog
>
Arnaldo Carvalho de Melo May 15, 2023, 8:54 p.m. UTC | #2 
Em Mon, May 15, 2023 at 10:09:08AM -0700, Ian Rogers escreveu:
> On Wed, May 10, 2023 at 11:27 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > The hists__find_annotations() allow to move to next or previous symbols
> > for annotation using the arrow keys.  But TUI annotate_browser__run()
> > uses the RIGHT key as ENTER to handle jump/call instructions.  That
> > makes the navigation to the next function impossible.
> >
> > I'd like to change it back to move the next symbol but I'm afraid if
> > some users get confused.  So I added a new pair of keys to handle that.
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> 
> Acked-by: Ian Rogers <irogers@google.com>
> 
> Thanks,
> Ian
> 
> > ---
> >  tools/perf/builtin-annotate.c     | 4 +++-
> >  tools/perf/ui/browsers/annotate.c | 4 +++-
> >  2 files changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> > index 63cdf6ea6f6d..425a7e2fd6fb 100644
> > --- a/tools/perf/builtin-annotate.c
> > +++ b/tools/perf/builtin-annotate.c
> > @@ -342,7 +342,7 @@ static void hists__find_annotations(struct hists *hists,
> >                 notes = symbol__annotation(he->ms.sym);
> >                 if (notes->src == NULL) {
> >  find_next:
> > -                       if (key == K_LEFT)
> > +                       if (key == K_LEFT || key == '<')
> >                                 nd = rb_prev(nd);
> >                         else
> >                                 nd = rb_next(nd);
> > @@ -378,9 +378,11 @@ static void hists__find_annotations(struct hists *hists,
> >                                         return;
> >                                 /* fall through */
> >                         case K_RIGHT:
> > +                       case '>':
> >                                 next = rb_next(nd);
> >                                 break;
> >                         case K_LEFT:
> > +                       case '<':
> >                                 next = rb_prev(nd);
> >                                 break;
> >                         default:
> > diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> > index 12c3ce530e42..70bad42b807b 100644
> > --- a/tools/perf/ui/browsers/annotate.c
> > +++ b/tools/perf/ui/browsers/annotate.c
> > @@ -781,9 +781,9 @@ static int annotate_browser__run(struct annotate_browser *browser,
> >                         ui_browser__help_window(&browser->b,
> >                 "UP/DOWN/PGUP\n"
> >                 "PGDN/SPACE    Navigate\n"
> > +               "</>           Move to prev/next symbol\n"
> >                 "q/ESC/CTRL+C  Exit\n\n"
> >                 "ENTER         Go to target\n"
> > -               "ESC           Exit\n"

I think the two above were unintentional? I'm removing this hunk.

- Arnaldo

> >                 "H             Go to hottest instruction\n"
> >                 "TAB/shift+TAB Cycle thru hottest instructions\n"
> >                 "j             Toggle showing jump to target arrows\n"
> > @@ -913,6 +913,8 @@ static int annotate_browser__run(struct annotate_browser *browser,
> >                         annotation__toggle_full_addr(notes, ms);
> >                         continue;
> >                 case K_LEFT:
> > +               case '<':
> > +               case '>':
> >                 case K_ESC:
> >                 case 'q':
> >                 case CTRL('c'):
> > --
> > 2.40.1.521.gf1e218fcd8-goog
> >
Arnaldo Carvalho de Melo May 15, 2023, 8:55 p.m. UTC | #3 
Em Mon, May 15, 2023 at 05:54:19PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Mon, May 15, 2023 at 10:09:08AM -0700, Ian Rogers escreveu:
> > On Wed, May 10, 2023 at 11:27 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > >
> > > The hists__find_annotations() allow to move to next or previous symbols
> > > for annotation using the arrow keys.  But TUI annotate_browser__run()
> > > uses the RIGHT key as ENTER to handle jump/call instructions.  That
> > > makes the navigation to the next function impossible.
> > >
> > > I'd like to change it back to move the next symbol but I'm afraid if
> > > some users get confused.  So I added a new pair of keys to handle that.
> > >
> > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > 
> > Acked-by: Ian Rogers <irogers@google.com>
> > 
> > Thanks,
> > Ian
> > 
> > > ---
> > >  tools/perf/builtin-annotate.c     | 4 +++-
> > >  tools/perf/ui/browsers/annotate.c | 4 +++-
> > >  2 files changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> > > index 63cdf6ea6f6d..425a7e2fd6fb 100644
> > > --- a/tools/perf/builtin-annotate.c
> > > +++ b/tools/perf/builtin-annotate.c
> > > @@ -342,7 +342,7 @@ static void hists__find_annotations(struct hists *hists,
> > >                 notes = symbol__annotation(he->ms.sym);
> > >                 if (notes->src == NULL) {
> > >  find_next:
> > > -                       if (key == K_LEFT)
> > > +                       if (key == K_LEFT || key == '<')
> > >                                 nd = rb_prev(nd);
> > >                         else
> > >                                 nd = rb_next(nd);
> > > @@ -378,9 +378,11 @@ static void hists__find_annotations(struct hists *hists,
> > >                                         return;
> > >                                 /* fall through */
> > >                         case K_RIGHT:
> > > +                       case '>':
> > >                                 next = rb_next(nd);
> > >                                 break;
> > >                         case K_LEFT:
> > > +                       case '<':
> > >                                 next = rb_prev(nd);
> > >                                 break;
> > >                         default:
> > > diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> > > index 12c3ce530e42..70bad42b807b 100644
> > > --- a/tools/perf/ui/browsers/annotate.c
> > > +++ b/tools/perf/ui/browsers/annotate.c
> > > @@ -781,9 +781,9 @@ static int annotate_browser__run(struct annotate_browser *browser,
> > >                         ui_browser__help_window(&browser->b,
> > >                 "UP/DOWN/PGUP\n"
> > >                 "PGDN/SPACE    Navigate\n"
> > > +               "</>           Move to prev/next symbol\n"
> > >                 "q/ESC/CTRL+C  Exit\n\n"
> > >                 "ENTER         Go to target\n"
> > > -               "ESC           Exit\n"
> 
> I think the two above were unintentional? I'm removing this hunk.

Humm I see, its a dup, will keep it.

- Arnaldo
diff mbox series

Patch

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 63cdf6ea6f6d..425a7e2fd6fb 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -342,7 +342,7 @@  static void hists__find_annotations(struct hists *hists,
 		notes = symbol__annotation(he->ms.sym);
 		if (notes->src == NULL) {
 find_next:
-			if (key == K_LEFT)
+			if (key == K_LEFT || key == '<')
 				nd = rb_prev(nd);
 			else
 				nd = rb_next(nd);
@@ -378,9 +378,11 @@  static void hists__find_annotations(struct hists *hists,
 					return;
 				/* fall through */
 			case K_RIGHT:
+			case '>':
 				next = rb_next(nd);
 				break;
 			case K_LEFT:
+			case '<':
 				next = rb_prev(nd);
 				break;
 			default:
diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index 12c3ce530e42..70bad42b807b 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -781,9 +781,9 @@  static int annotate_browser__run(struct annotate_browser *browser,
 			ui_browser__help_window(&browser->b,
 		"UP/DOWN/PGUP\n"
 		"PGDN/SPACE    Navigate\n"
+		"</>           Move to prev/next symbol\n"
 		"q/ESC/CTRL+C  Exit\n\n"
 		"ENTER         Go to target\n"
-		"ESC           Exit\n"
 		"H             Go to hottest instruction\n"
 		"TAB/shift+TAB Cycle thru hottest instructions\n"
 		"j             Toggle showing jump to target arrows\n"
@@ -913,6 +913,8 @@  static int annotate_browser__run(struct annotate_browser *browser,
 			annotation__toggle_full_addr(notes, ms);
 			continue;
 		case K_LEFT:
+		case '<':
+		case '>':
 		case K_ESC:
 		case 'q':
 		case CTRL('c'):