Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] kconfig: Create links to main menu items in search
To:     Masahiro Yamada <masahiroy@kernel.org>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux Kbuild mailing list <linux-kbuild@vger.kernel.org>
References: <20210901175326.568535-1-arielmarcovitch@gmail.com>
 <CAK7LNAS2AA=0UxJudhuxmRJkWfXb2=fELc9cA6eH+cvSkOc=7Q@mail.gmail.com>
From:   Ariel Marcovitch <arielmarcovitch@gmail.com>
Message-ID: <e4c0d198-0ae6-e415-51b5-0a33ff4fb82e@gmail.com>
Date:   Sat, 11 Sep 2021 21:18:24 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.13.0
MIME-Version: 1.0
In-Reply-To: <CAK7LNAS2AA=0UxJudhuxmRJkWfXb2=fELc9cA6eH+cvSkOc=7Q@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Precedence: bulk

On 10/09/2021 5:14, Masahiro Yamada wrote:
>On Thu, Sep 2, 2021 at 2:53 AM Ariel Marcovitch
><arielmarcovitch@gmail.com> 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@gmail.com>
>>---
>>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