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@xxxxxxxxxxxxxxx
> Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
> ---
> 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
