Re: [PATCH] kconfig qconf: Add grey background for hidden options

[Maxime Chretien]

On 07/08/2020 15:54, Masahiro Yamada wrote:
> On Wed, Jul 8, 2020 at 10:30 PM Maxime Chretien
> <maxime.chretien@xxxxxxxxxxx>  wrote:
> > This is useful to see which configuration parameters can be edited
> > or not when "Show All Options" is enabled.
> >
> > Signed-off-by: Maxime Chretien<maxime.chretien@xxxxxxxxxxx>
> > ---
> >   scripts/kconfig/qconf.cc | 7 +++++++
> >   scripts/kconfig/qconf.h  | 4 ++++
> >   2 files changed, 11 insertions(+)
> I like the idea, but
> maybe this patch could be improved?
>
> For example, in the following test code,
> BAR is correctly painted grey when CONFIG_FOO=n,
> but "my menu" is always white despite of
> "depends on FOO"
>
> ----(test code)------
>
> config FOO
>         bool "foo"
>
> config BAR
>         bool "bar"
>         depends on FOO
>
> menu "my menu"
>         depends on FOO
>
> endmenu
>
> -------(test code end)----


It could probably be improved.
Maybe the "visible" state of the menu can be changed
or maybe we should use another element in the test
to grey out menus that needs to be greyed out.


> > diff --git a/scripts/kconfig/qconf.cc b/scripts/kconfig/qconf.cc
> > index c0ac8f7b5f1a..be9ff4651da1 100644
> > --- a/scripts/kconfig/qconf.cc
> > +++ b/scripts/kconfig/qconf.cc
> > @@ -208,6 +208,13 @@ void ConfigItem::updateMenu(void)
> >          }
> >          if (!sym_has_value(sym) && visible)
> >                  prompt += " (NEW)";
> > +
> > +       if(!visible) {
> > +               setBackground(promptColIdx, QBrush(QColor("#E0E0E0")));
> > +       } else {
> > +               setBackground(promptColIdx, QBrush());
> > +       }
> > +
> I think all the columns should be grey-grounded.
> Please note you can click other columns to
> toggle y/m/n.
>
>
> How about something like this?
>
>
>
> QBrush brush;
>
> if (visible)
>          brush = QBrush()
> else
>          brush = QBrush(QColor("#E0E0E0"));
>
> setBackground(promptColIdx, brush);
> setBackground(nameColIdx, brush);
> setBackground(noColIdx, brush);
> setBackground(modColIdx, brush);
> setBackground(yesColIdx, brush);
> setBackground(dataColIdx, brush);


Yes I agree should be better like that so that we can
see what's grey or not even if we have scrolled far
from the first column.


> >   set_prompt:
> >          setText(promptColIdx, prompt);
> >   }
> > diff --git a/scripts/kconfig/qconf.h b/scripts/kconfig/qconf.h
> > index c879d79ce817..79e47e8c1ae7 100644
> > --- a/scripts/kconfig/qconf.h
> > +++ b/scripts/kconfig/qconf.h
> > @@ -174,6 +174,10 @@ class ConfigItem : public QTreeWidgetItem {
> >          {
> >                  return Parent::text(idx);
> >          }
> > +       void setBackground(colIdx idx, const QBrush& brush)
> > +       {
> > +               Parent::setBackground(idx, brush);
> > +       }
> I do not understand why this wrapper is useful...


Well we need to use "Parent" to have access to the "setBackground" function
and I thought it was the way to use it has it's done with "setPixmap" 
just under it.

Maybe there is a better way to do that, I'm a beginner in linux development
so I'm not totally aware of all the rules and ways to add new things.


> >          void setPixmap(colIdx idx, const QIcon &icon)
> >          {
> >                  Parent::setIcon(idx, icon);
> > --
> > 2.27.0


Thanks for reviewing this.

Best regards
Maxime Chretien