Re: [PATCH] kconfig: Create links to main menu items in search

[Ariel Marcovitch]

On 10/09/2021 5:14, Masahiro Yamada wrote:
> On Thu, Sep 2, 2021 at 2:53 AM Ariel Marcovitch
> <arielmarcovitch@xxxxxxxxx> wrote:
> > When one searches for a main menu item, links aren't created for it like
> > with the rest of the symbols.
> >
> > This happens because we trace the item until we get to the rootmenu, but
> > we don't include it in the path of the item. The rationale was probably
> > that we don't want to show the main menu in the path of all items,
> > because it is redundant.
> >
> > However, when an item has only the rootmenu in its path it should be
> > included, because this way the user can jump to its location.
> >
> > In case the item is a direct child of the rootmenu, show it in the
> > 'Location:' section as 'Main Menu'.
> >
> > This makes the 'if (i > 0)' superfluous because each item with prompt
> > will have at least one menu in its path.
> >
> > Signed-off-by: Ariel Marcovitch <arielmarcovitch@xxxxxxxxx>
> > ---
> > scripts/kconfig/menu.c | 40 ++++++++++++++++++++++++++--------------
> > 1 file changed, 26 insertions(+), 14 deletions(-)
> >
> > diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
> > index 606ba8a63c24..8d7e3b07bf93 100644
> > --- a/scripts/kconfig/menu.c
> > +++ b/scripts/kconfig/menu.c
> > @@ -712,6 +712,7 @@ static void get_prompt_str(struct gstr *r, struct 
property *prop,
> > int i, j;
> > struct menu *submenu[8], *menu, *location = NULL;
> > struct jump_key *jump = NULL;
> > +const char *prompt = NULL;
>
>
> Can you move this to the for-loop ?
>
> The initializer is unneeded.
>
>
> > str_printf(r, " Prompt: %s\n", prop->text);
> >
> > @@ -735,6 +736,13 @@ static void get_prompt_str(struct gstr *r, struct 
property *prop,
> > if (location == NULL && accessible)
> > location = menu;
> > }
> > +
> > +/* If we have only the root menu, show it */
> > +if (i == 0) {
> > +location = &rootmenu;
> > +submenu[i++] = location;
> > +}
>
>
> Instead of handling this as a special case,
> can we include the rootmenu all the time?
>
> We can change the for-loop condition to:
>
> for (i = 0; menu && i < 8; menu = menu->parent) {
Of course.
However, it means search entries will get a bit larger. I guess it is
worth it?
> > if (head && location) {
> > jump = xmalloc(sizeof(struct jump_key));
> >
> > @@ -758,21 +766,25 @@ static void get_prompt_str(struct gstr *r, struct 
property *prop,
> > list_add_tail(&jump->entries, head);
> > }
> >
> > -if (i > 0) {
> > -str_printf(r, " Location:\n");
> > -for (j = 4; --i >= 0; j += 2) {
> > -menu = submenu[i];
> > -if (jump && menu == location)
> > -jump->offset = strlen(r->s);
> > -str_printf(r, "%*c-> %s", j, ' ',
> > -menu_get_prompt(menu));
> > -if (menu->sym) {
> > -str_printf(r, " (%s [=%s])", menu->sym->name ?
> > -menu->sym->name : "<choice>",
> > -sym_get_string_value(menu->sym));
> > -}
> > -str_append(r, "\n");
> > +str_printf(r, " Location:\n");
> > +for (j = 4; --i >= 0; j += 2) {
> > +menu = submenu[i];
> > +if (jump && menu == location)
> > +jump->offset = strlen(r->s);
> > +
> > +/* The real rootmenu prompt is ugly */
> > +if (menu == &rootmenu)
> > +prompt = "Main Menu";
>
> Can you use "Main menu" for the consistency
> with scripts/kconfig/parser.y line 501?
Seems reasonable. Just to clarify, the prompt there is not relevant
for linux's Kconfig right?
> > +else
> > +prompt = menu_get_prompt(menu);
>
>
> I think it is better to omit '->' for the rootmenu.
>
>
> if (menu == &rootmenu) {
> prompt = "Main menu";
> marker = "";
> } else {
> prompt = menu_get_prompt(menu);
> marker = "->";
> }
>
> str_printf(r, "%*c%s %s", j, ' ', marker, prompt);
Perhaps it will be better to split to separate printfs? I think it
will be cleaner. It will make the extra vars unneeded as well.
Also, the results look a bit weird... maybe we should use a different
marker for rootmenu, like '--' or something?
> Maybe, this will make the help look cleaner.
>
>
>
>
>
> > +str_printf(r, "%*c-> %s", j, ' ', prompt);
> > +if (menu->sym) {
> > +str_printf(r, " (%s [=%s])", menu->sym->name ?
> > +menu->sym->name : "<choice>",
> > +sym_get_string_value(menu->sym));
> > }
> > +str_append(r, "\n");
> > }
> > }
> >
> >
> > base-commit: 087e856cfb76e9eef9a3a6e000854794f3c36e24
> > --
> > 2.25.1
>
>
> --
> Best Regards
>
> Masahiro Yamada
Thanks for the comments :)
Ariel Marcovitch