On Tue, Mar 5, 2024 at 11:15 PM Masahiro Yamada <masahiroy@xxxxxxxxxx> wrote: > > On Tue, Mar 5, 2024 at 4:47 PM Tomasz Figa <tfiga@xxxxxxxxxxxx> wrote: > > > > On Thu, Feb 29, 2024 at 10:57 AM Masahiro Yamada <masahiroy@xxxxxxxxxx> wrote: > > > > > > On Thu, Feb 29, 2024 at 1:36 AM Matthew Bystrin <dev.mbstr@xxxxxxxxx> wrote: > > > > > > > > On Wed Feb 28, 2024 at 9:00 AM MSK, Tomasz Figa wrote: > > > > > When hidden options are toggled on (using 'z'), the number of options > > > > > on the screen can be overwhelming and may make it hard to distinguish > > > > > between available and hidden ones. Make them easier to distinguish by > > > > > displaying the hidden one with a different color (COLOR_YELLOW for color > > > > > themes and A_DIM for mono). > > > > > > > > > > Signed-off-by: Tomasz Figa <tfiga@xxxxxxxxxxxx> > > > > > --- > > > > > scripts/kconfig/lxdialog/dialog.h | 5 +++++ > > > > > scripts/kconfig/lxdialog/menubox.c | 12 ++++++++---- > > > > > scripts/kconfig/lxdialog/util.c | 19 +++++++++++++++++++ > > > > > scripts/kconfig/mconf.c | 18 ++++++++++++++++++ > > > > > 4 files changed, 50 insertions(+), 4 deletions(-) > > > > > > > > > > Changes from v1: > > > > > (https://patchwork.kernel.org/project/linux-kbuild/patch/20231228054630.3595093-1-tfiga@xxxxxxxxxxxx/) > > > > > * Replaced A_DIM for color themes with COLOR_YELLOW, because the former > > > > > has no effect to black text on some commonly used terminals, e.g. > > > > > gnome-terminal, foot. Reported by Masahiro Yamada and Nicolas Schier. > > > > > I ended up with COLOR_YELLOW, as it seems to look comparatively dim > > > > > with mutliple light and dark color themes in Chromium hterm and > > > > > gnome-terminal. > > > > > > > > Thanks! Run a quick tests in xterm. Looks neat! > > > > > > > > Is there a reason to set hidden flag in all of the _if_ and _switch_ statements > > > > in the build_conf() function? Could similar be done in a more generic way? For > > > > example: > > > > > > > > visible = menu_is_visible(menu); > > > > if (!visible) > > > > item_set_hidden(TRUE); > > > > > > > > Or this approach will bring some negative side effects ? > > > > > > > > > > > > > I guess he just inserted item_set_hidden() where he saw item_make(). > > > > > > > > > Since build_conf() resources to itself, the code flow > > > is difficult to track. > > > > > > > > > You can safely factor it out in some places (for example, just blow), > > > but that does not make a big difference. > > > > > > > > > > > > diff --git a/scripts/kconfig/mconf.c b/scripts/kconfig/mconf.c > > > index b7e08ec98717..ba0f177121ed 100644 > > > --- a/scripts/kconfig/mconf.c > > > +++ b/scripts/kconfig/mconf.c > > > @@ -546,16 +546,15 @@ static void build_conf(struct menu *menu) > > > } > > > item_set_tag('t'); > > > item_set_data(menu); > > > - if (!visible) > > > - item_set_hidden(TRUE); > > > } else { > > > item_make(" "); > > > item_set_tag(def_menu ? 't' : ':'); > > > item_set_data(menu); > > > - if (!visible) > > > - item_set_hidden(TRUE); > > > > I wanted to be consistent with the current code, which already has the > > same item_set_data(menu) in both branches. I'm okay with either. Do > > you want me to resend with this change? > > > > I am fine with either way. > I thought build_conf() was messy, > but it is not a problem with your patch. > > > > Talking about the code consistency, what about this suggestion? > > https://lore.kernel.org/linux-kbuild/CAAFQd5AOvUtHOOU-OKQKJwyJGXSt6EopcMBsHWz83n_0XfnOjA@xxxxxxxxxxxxxx/T/#med19d030bf8167637964b58da0f5b86b18fe3f5e > > > In line 26, A_DIM is directly assigned. > > dlg.button_inactive.atr = A_DIM; > > > > If you verbosely add 'A_NORMAL |', > the other line should look like this: > > > dlg.item_hidden_selected.atr = A_NORMAL | A_REVERSE | A_DIM; > > > And, we need to insert 'A_NORMAL |' > to every assignment. > > > > If you agree with me, you can offer to modify the patch locally. > Ah, sorry, missed that email. I think we can just remove A_NORMAL if we have the other bits, as you suggested in your first reply. Let me fix that. Best regards, Tomasz
