[PATCH 1/2] perf hists browser: Fix hierarchy mode header

[PATCH 1/2] perf hists browser: Fix hierarchy mode header

[Namhyung Kim]

The commit ef9ff6017e3c4 ("perf ui browser: Move the extra title lines
from the hists browser") introduced ui_browser__gotorc_title() to help
moving non-title lines easily.  But it missed to update the title for
the hierarchy mode so it won't print the header line on TUI at all.

  $ perf report --hierarchy

Fixes: ef9ff6017e3c4 ("perf ui browser: Move the extra title lines from the hists browser")
Cc: stable@vger.kernel.org
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/browsers/hists.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index c7ad9e003080..d8b88f10a48d 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1779,7 +1779,7 @@ static void hists_browser__hierarchy_headers(struct hist_browser *browser)
 	hists_browser__scnprintf_hierarchy_headers(browser, headers,
 						   sizeof(headers));
 
-	ui_browser__gotorc(&browser->b, 0, 0);
+	ui_browser__gotorc_title(&browser->b, 0, 0);
 	ui_browser__set_color(&browser->b, HE_COLORSET_ROOT);
 	ui_browser__write_nstring(&browser->b, headers, browser->b.width + 1);
 }
-- 
2.41.0.487.g6d72f3e995-goog

[PATCH 2/2] perf hists browser: Fix the number of entries for 'e' key

[Namhyung Kim]

The 'e' key is to toglle expand/collapse the selected entry only.  But
the current code has a bug that it only increases the number of entries
by 1 in the hierarchy mode so users cannot move under the current entry
after the key stroke.  This is due to a wrong assumption in the
hist_entry__set_folding().

The commit b33f922651011 ("perf hists browser: Put hist_entry folding
logic into single function") factored out the code, but actually it
should be handled separately.  The hist_browser__set_folding() is to
update fold state for each entry so it needs to traverse all (child)
entries regardless of the current fold state.  So it increases the
number of entries by 1.

But the hist_entry__set_folding() only cares the currently selected
entry and its all children.  So it should count all unfolded child
entries.  This code is implemented in hist_browser__toggle_fold()
already so we can just call it.

Fixes: b33f922651011 ("perf hists browser: Put hist_entry folding logic into single function")
Cc: stable@vger.kernel.org
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/browsers/hists.c | 58 ++++++++++++++--------------------
 1 file changed, 24 insertions(+), 34 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index d8b88f10a48d..70db5a717905 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -407,11 +407,6 @@ static bool hist_browser__selection_has_children(struct hist_browser *browser)
 	return container_of(ms, struct callchain_list, ms)->has_children;
 }
 
-static bool hist_browser__he_selection_unfolded(struct hist_browser *browser)
-{
-	return browser->he_selection ? browser->he_selection->unfolded : false;
-}
-
 static bool hist_browser__selection_unfolded(struct hist_browser *browser)
 {
 	struct hist_entry *he = browser->he_selection;
@@ -584,8 +579,8 @@ static int hierarchy_set_folding(struct hist_browser *hb, struct hist_entry *he,
 	return n;
 }
 
-static void __hist_entry__set_folding(struct hist_entry *he,
-				      struct hist_browser *hb, bool unfold)
+static void hist_entry__set_folding(struct hist_entry *he,
+				    struct hist_browser *hb, bool unfold)
 {
 	hist_entry__init_have_children(he);
 	he->unfolded = unfold ? he->has_children : false;
@@ -603,34 +598,12 @@ static void __hist_entry__set_folding(struct hist_entry *he,
 		he->nr_rows = 0;
 }
 
-static void hist_entry__set_folding(struct hist_entry *he,
-				    struct hist_browser *browser, bool unfold)
-{
-	double percent;
-
-	percent = hist_entry__get_percent_limit(he);
-	if (he->filtered || percent < browser->min_pcnt)
-		return;
-
-	__hist_entry__set_folding(he, browser, unfold);
-
-	if (!he->depth || unfold)
-		browser->nr_hierarchy_entries++;
-	if (he->leaf)
-		browser->nr_callchain_rows += he->nr_rows;
-	else if (unfold && !hist_entry__has_hierarchy_children(he, browser->min_pcnt)) {
-		browser->nr_hierarchy_entries++;
-		he->has_no_entry = true;
-		he->nr_rows = 1;
-	} else
-		he->has_no_entry = false;
-}
-
 static void
 __hist_browser__set_folding(struct hist_browser *browser, bool unfold)
 {
 	struct rb_node *nd;
 	struct hist_entry *he;
+	double percent;
 
 	nd = rb_first_cached(&browser->hists->entries);
 	while (nd) {
@@ -640,6 +613,21 @@ __hist_browser__set_folding(struct hist_browser *browser, bool unfold)
 		nd = __rb_hierarchy_next(nd, HMD_FORCE_CHILD);
 
 		hist_entry__set_folding(he, browser, unfold);
+
+		percent = hist_entry__get_percent_limit(he);
+		if (he->filtered || percent < browser->min_pcnt)
+			continue;
+
+		if (!he->depth || unfold)
+			browser->nr_hierarchy_entries++;
+		if (he->leaf)
+			browser->nr_callchain_rows += he->nr_rows;
+		else if (unfold && !hist_entry__has_hierarchy_children(he, browser->min_pcnt)) {
+			browser->nr_hierarchy_entries++;
+			he->has_no_entry = true;
+			he->nr_rows = 1;
+		} else
+			he->has_no_entry = false;
 	}
 }
 
@@ -659,8 +647,10 @@ static void hist_browser__set_folding_selected(struct hist_browser *browser, boo
 	if (!browser->he_selection)
 		return;
 
-	hist_entry__set_folding(browser->he_selection, browser, unfold);
-	browser->b.nr_entries = hist_browser__nr_entries(browser);
+	if (unfold == browser->he_selection->unfolded)
+		return;
+
+	hist_browser__toggle_fold(browser);
 }
 
 static void ui_browser__warn_lost_events(struct ui_browser *browser)
@@ -732,8 +722,8 @@ static int hist_browser__handle_hotkey(struct hist_browser *browser, bool warn_l
 		hist_browser__set_folding(browser, true);
 		break;
 	case 'e':
-		/* Expand the selected entry. */
-		hist_browser__set_folding_selected(browser, !hist_browser__he_selection_unfolded(browser));
+		/* Toggle expand/collapse the selected entry. */
+		hist_browser__toggle_fold(browser);
 		break;
 	case 'H':
 		browser->show_headers = !browser->show_headers;
-- 
2.41.0.487.g6d72f3e995-goog

Re: [PATCH 2/2] perf hists browser: Fix the number of entries for 'e' key

[Arnaldo Carvalho de Melo]

Em Mon, Jul 31, 2023 at 02:49:33AM -0700, Namhyung Kim escreveu:
> The 'e' key is to toglle expand/collapse the selected entry only.  But
> the current code has a bug that it only increases the number of entries
> by 1 in the hierarchy mode so users cannot move under the current entry
> after the key stroke.  This is due to a wrong assumption in the
> hist_entry__set_folding().
> 
> The commit b33f922651011 ("perf hists browser: Put hist_entry folding
> logic into single function") factored out the code, but actually it
> should be handled separately.  The hist_browser__set_folding() is to
> update fold state for each entry so it needs to traverse all (child)
> entries regardless of the current fold state.  So it increases the
> number of entries by 1.
> 
> But the hist_entry__set_folding() only cares the currently selected
> entry and its all children.  So it should count all unfolded child
> entries.  This code is implemented in hist_browser__toggle_fold()
> already so we can just call it.

Thanks, tested and applied.

- Arnaldo

 
> Fixes: b33f922651011 ("perf hists browser: Put hist_entry folding logic into single function")
> Cc: stable@vger.kernel.org
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/ui/browsers/hists.c | 58 ++++++++++++++--------------------
>  1 file changed, 24 insertions(+), 34 deletions(-)
> 
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index d8b88f10a48d..70db5a717905 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -407,11 +407,6 @@ static bool hist_browser__selection_has_children(struct hist_browser *browser)
>  	return container_of(ms, struct callchain_list, ms)->has_children;
>  }
>  
> -static bool hist_browser__he_selection_unfolded(struct hist_browser *browser)
> -{
> -	return browser->he_selection ? browser->he_selection->unfolded : false;
> -}
> -
>  static bool hist_browser__selection_unfolded(struct hist_browser *browser)
>  {
>  	struct hist_entry *he = browser->he_selection;
> @@ -584,8 +579,8 @@ static int hierarchy_set_folding(struct hist_browser *hb, struct hist_entry *he,
>  	return n;
>  }
>  
> -static void __hist_entry__set_folding(struct hist_entry *he,
> -				      struct hist_browser *hb, bool unfold)
> +static void hist_entry__set_folding(struct hist_entry *he,
> +				    struct hist_browser *hb, bool unfold)
>  {
>  	hist_entry__init_have_children(he);
>  	he->unfolded = unfold ? he->has_children : false;
> @@ -603,34 +598,12 @@ static void __hist_entry__set_folding(struct hist_entry *he,
>  		he->nr_rows = 0;
>  }
>  
> -static void hist_entry__set_folding(struct hist_entry *he,
> -				    struct hist_browser *browser, bool unfold)
> -{
> -	double percent;
> -
> -	percent = hist_entry__get_percent_limit(he);
> -	if (he->filtered || percent < browser->min_pcnt)
> -		return;
> -
> -	__hist_entry__set_folding(he, browser, unfold);
> -
> -	if (!he->depth || unfold)
> -		browser->nr_hierarchy_entries++;
> -	if (he->leaf)
> -		browser->nr_callchain_rows += he->nr_rows;
> -	else if (unfold && !hist_entry__has_hierarchy_children(he, browser->min_pcnt)) {
> -		browser->nr_hierarchy_entries++;
> -		he->has_no_entry = true;
> -		he->nr_rows = 1;
> -	} else
> -		he->has_no_entry = false;
> -}
> -
>  static void
>  __hist_browser__set_folding(struct hist_browser *browser, bool unfold)
>  {
>  	struct rb_node *nd;
>  	struct hist_entry *he;
> +	double percent;
>  
>  	nd = rb_first_cached(&browser->hists->entries);
>  	while (nd) {
> @@ -640,6 +613,21 @@ __hist_browser__set_folding(struct hist_browser *browser, bool unfold)
>  		nd = __rb_hierarchy_next(nd, HMD_FORCE_CHILD);
>  
>  		hist_entry__set_folding(he, browser, unfold);
> +
> +		percent = hist_entry__get_percent_limit(he);
> +		if (he->filtered || percent < browser->min_pcnt)
> +			continue;
> +
> +		if (!he->depth || unfold)
> +			browser->nr_hierarchy_entries++;
> +		if (he->leaf)
> +			browser->nr_callchain_rows += he->nr_rows;
> +		else if (unfold && !hist_entry__has_hierarchy_children(he, browser->min_pcnt)) {
> +			browser->nr_hierarchy_entries++;
> +			he->has_no_entry = true;
> +			he->nr_rows = 1;
> +		} else
> +			he->has_no_entry = false;
>  	}
>  }
>  
> @@ -659,8 +647,10 @@ static void hist_browser__set_folding_selected(struct hist_browser *browser, boo
>  	if (!browser->he_selection)
>  		return;
>  
> -	hist_entry__set_folding(browser->he_selection, browser, unfold);
> -	browser->b.nr_entries = hist_browser__nr_entries(browser);
> +	if (unfold == browser->he_selection->unfolded)
> +		return;
> +
> +	hist_browser__toggle_fold(browser);
>  }
>  
>  static void ui_browser__warn_lost_events(struct ui_browser *browser)
> @@ -732,8 +722,8 @@ static int hist_browser__handle_hotkey(struct hist_browser *browser, bool warn_l
>  		hist_browser__set_folding(browser, true);
>  		break;
>  	case 'e':
> -		/* Expand the selected entry. */
> -		hist_browser__set_folding_selected(browser, !hist_browser__he_selection_unfolded(browser));
> +		/* Toggle expand/collapse the selected entry. */
> +		hist_browser__toggle_fold(browser);
>  		break;
>  	case 'H':
>  		browser->show_headers = !browser->show_headers;
> -- 
> 2.41.0.487.g6d72f3e995-goog
> 

-- 

- Arnaldo

Re: [PATCH 1/2] perf hists browser: Fix hierarchy mode header

[Arnaldo Carvalho de Melo]

Em Mon, Jul 31, 2023 at 02:49:32AM -0700, Namhyung Kim escreveu:
> The commit ef9ff6017e3c4 ("perf ui browser: Move the extra title lines
> from the hists browser") introduced ui_browser__gotorc_title() to help
> moving non-title lines easily.  But it missed to update the title for
> the hierarchy mode so it won't print the header line on TUI at all.
> 
>   $ perf report --hierarchy

Thanks, tested and applied.

- Arnaldo

 
> Fixes: ef9ff6017e3c4 ("perf ui browser: Move the extra title lines from the hists browser")
> Cc: stable@vger.kernel.org
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/ui/browsers/hists.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index c7ad9e003080..d8b88f10a48d 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -1779,7 +1779,7 @@ static void hists_browser__hierarchy_headers(struct hist_browser *browser)
>  	hists_browser__scnprintf_hierarchy_headers(browser, headers,
>  						   sizeof(headers));
>  
> -	ui_browser__gotorc(&browser->b, 0, 0);
> +	ui_browser__gotorc_title(&browser->b, 0, 0);
>  	ui_browser__set_color(&browser->b, HE_COLORSET_ROOT);
>  	ui_browser__write_nstring(&browser->b, headers, browser->b.width + 1);
>  }
> -- 
> 2.41.0.487.g6d72f3e995-goog
> 

-- 

- Arnaldo