| Message ID | 20200708133015.12286-1-maxime.chretien@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kconfig qconf: Add grey background for hidden options | expand |
On Wed, Jul 8, 2020 at 10:30 PM Maxime Chretien <maxime.chretien@bootlin.com> 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@bootlin.com> > --- > 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)---- > 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); > 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... > void setPixmap(colIdx idx, const QIcon &icon) > { > Parent::setIcon(idx, icon); > -- > 2.27.0 >
On 07/08/2020 15:54, Masahiro Yamada wrote: > On Wed, Jul 8, 2020 at 10:30 PM Maxime Chretien > <maxime.chretien@bootlin.com> 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@bootlin.com> >> --- >> 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
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()); + } + 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); + } void setPixmap(colIdx idx, const QIcon &icon) { Parent::setIcon(idx, icon);
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@bootlin.com> --- scripts/kconfig/qconf.cc | 7 +++++++ scripts/kconfig/qconf.h | 4 ++++ 2 files changed, 11 insertions(+)