menuconfig: distinguish between selected-by-another options and comments

menuconfig currently represents options implied by another option ('select' directive in Kconfig) by prefixing them with '---'. Unfortunately the same notation is used for comments.

This patch changes notation of selected-by-another items by introducing 2 new representations for implied options:
{*} or {M} for options selected by another modularized one, thus builtin or module capable,
-*- or -M- for options that cannot be at the moment changed by user.

The idea is to represent actual capability of the option by braces (dashes) around and to always report actual state by * or M inside.

---
Changes since v1:
* introduce sym_get_minimal_value(), so that access to struct symbol is abstracted.
* change also menuconfig's window header text to reflect the change. I'm still not sure if the wording is optimal.

v1 was acked by Randy Dunlap, I'm not sure if it's appropriate to keep Acked-by when patch changes. (teach me please)

 scripts/kconfig/lkc_proto.h |    1 +
 scripts/kconfig/mconf.c     |   21 +++++++++++++--------
 scripts/kconfig/symbol.c    |    5 +++++
 3 files changed, 19 insertions(+), 8 deletions(-)

index 4d09f6d..98642bc 100644
@@ -34,6 +34,7 @@ P(sym_string_valid,bool,(struct symbol *sym, const char 

*newval));
P(sym_string_within_range,bool,(struct symbol *sym, const char *str)); P(sym_set_string_value,bool,(struct symbol *sym, const char *newval)); P(sym_is_changable,bool,(struct symbol *sym));
+P(sym_get_minimal_value,tristate,(struct symbol *sym));
P(sym_get_choice_prop,struct property *,(struct symbol *sym)); P(sym_get_default_prop,struct property *,(struct symbol *sym)); P(sym_get_string_value,const char *,(struct symbol *sym));

index bc5854e..77ab552 100644
@@ -35,9 +35,13 @@ static const char mconf_readme[] = N_(
 "kernel parameters which are not really features, but must be\n"
 "entered in as decimal or hexadecimal numbers or possibly text.\n"
 "\n"
-"Menu items beginning with [*], <M> or [ ] represent features\n"
-"configured to be built in, modularized or removed respectively.\n"
-"Pointed brackets <> represent module capable features.\n"
+"Menu items beginning with following braces represent features that\n"
+"  [ ] can be built in or removed\n"
+"  < > can be built in, modularized or removed\n"
+"  { } can be built in or modularized (selected by other feature)\n"
+"  - - are selected by other feature,\n"
+"while *, M or whitespace inside braces means to build in, build as\n"
+"a module or to exclude the feature respectively.\n"
 "\n"
 "To change any of these features, highlight it with the cursor\n"
 "keys and press <Y> to build it in, <M> to make it a module or\n"

@@ -178,9 +182,9 @@ menu_instructions[] = N_(
 	"Arrow keys navigate the menu.  "
 	"<Enter> selects submenus --->.  "
 	"Highlighted letters are hotkeys.  "
-	"Pressing <Y> includes, <N> excludes, <M> modularizes features.  "
+	"Pressing <Y> includes (*), <M> modularizes (M), <N> excludes features 

( ). "
"Press <Esc><Esc> to exit, <?> for Help, </> for Search. "
- "Legend: [*] built-in [ ] excluded <M> module < > module capable"),
+ "Legend: [*] built-in, < > module capable, { } implied, -*- forced."),
radiolist_instructions[] = N_(
"Use the arrow keys to navigate this window or "
"press the hotkey of the item you wish to select "
@@ -560,7 +564,7 @@ static void build_conf(struct menu *menu)
if (sym_is_changable(sym))
item_make("[%c]", val == no ? ' ' : '*');
else
- item_make("---");
+ item_make("-%c-", val == no ? ' ' : '*');
item_set_tag('t'); item_set_data(menu);
break;
@@ -571,9 +575,10 @@ static void build_conf(struct menu *menu)
default: ch = ' '; break;
}
if (sym_is_changable(sym))
- item_make("<%c>", ch);
+ item_make((sym_get_minimal_value(sym) == mod) ?
+ "{%c}" : "<%c>", ch);
else
- item_make("---");
+ item_make("-%c-", ch);
item_set_tag('t'); item_set_data(menu);
break;

index c35dcc5..b9832d0 100644
@@ -643,6 +643,11 @@ bool sym_is_changable(struct symbol *sym)
 	return sym->visible > sym->rev_dep.tri;
 }
 
+tristate sym_get_minimal_value(struct symbol *sym)
+{
+	return sym->rev_dep.tri;
+}
+
 struct symbol *sym_lookup(const char *name, int isconst)
 {
 	struct symbol *symbol;

1.5.1.6

Oh please reply to my address [email protected]. I mismatched Reply-To and In-Reply-To e-mail headers when trying to stick the message to original thread.

Sorry for the noise and inconvenience I caused.

This gets rid of the ambiguous ---, does not it? (If so, nice!)

Definately. You may try it, the patch is against current Linus's HEAD, but may apply cleanly also against 2.6.22 as none of the files changed since. (AFAIK, too lazy to check it)

Regards,
Matej

I don't see any problems with the wording other than I'm not very strongly in favor of the word "implied". I don't have a better word to use other than "selected", but that's just an implementation detail, not especially user friendly.

Good question. I think you did it correctly.

With this patch we can now distingush between comments and menulines that the user cannot cahnge.
As one of the major issues with kconfig is the evil select handling it would have been much preferred to distingush between lines that are selected-by and lines that cannot be changed for other reasons.
The block menu is a good example where we do not sue select. It is marked '---' because it is default y if EMBEDDED is not selected.

So the intention is clear and your changed interface (sym_get_min_val()) look OK.
But can you take a look at distingushing between non-selectable options due to dependency issues and seleted-by symbols.

Sam

Hi,

I shortly considered doing something like this, but initially I wanted to keep it simple. It's a compromise between overloading the user interface with information and keeping it simple. That's the only concern I have with it, but if everyone else likes I don't mind either. :)

bye, Roman

Hi,

Do you have an example in mind? If a symbol is not changable, but still visible, a select is usually involved.

bye, Roman

menuconfig currently represents options implied by another option ('select' directive in Kconfig) by prefixing them with '---'. Unfortunately the same notation is used for comments.

This patch changes notation of selected-by-another items by introducing new representations for implied options:
{*} or {M} for module capable selected-by features
(*) for module incapable selected-by features

Additionally introduce new notation for features unchangeable due to unsatisfied dependency of prompt field: "-*-" or "- -". (See CONFIG_BLOCK)

Add help text reporting why feature is unchangable.

---
Hi Sam, Randy, Roman,
I'm presenting third version of the patch, which should address all mentioned issues with the patch.
I'm not sure if the text in symbol's help is necessary, feel free to exclude it.

Changes since v2:
* introduce sym_get_maximal_value()
* make selected-by another features distinguishable from features with no active prompt, thus unchangeable.
* add text to each unchangeable symbol's help explaining why it's so.

Changes since v1:
* introduce sym_get_minimal_value(), so that access to struct symbol is abstracted.
* change also menuconfig's window header text to reflect the change. I'm still not sure if the wording is optimal.

 scripts/kconfig/lkc_proto.h |    2 +
 scripts/kconfig/mconf.c     |   55 +++++++++++++++++++++++++++++--------------
 scripts/kconfig/symbol.c    |   10 ++++++++
 3 files changed, 49 insertions(+), 18 deletions(-)

index 4d09f6d..c2e3461 100644
@@ -34,6 +34,8 @@ P(sym_string_valid,bool,(struct symbol *sym, const char *newval));
 P(sym_string_within_range,bool,(struct symbol *sym, const char *str));
 P(sym_set_string_value,bool,(struct symbol *sym, const char *newval));
 P(sym_is_changable,bool,(struct symbol *sym));
+P(sym_get_minimal_value,tristate,(struct symbol *sym));
+P(sym_get_maximal_value,tristate,(struct symbol *sym));
 P(sym_get_choice_prop,struct property *,(struct symbol *sym));
 P(sym_get_default_prop,struct property *,(struct symbol *sym));
 P(sym_get_string_value,const char *,(struct symbol *sym));

index c35dcc5..8fe1d72 100644
@@ -643,6 +643,16 @@ bool sym_is_changable(struct symbol *sym)
 	return sym->visible > sym->rev_dep.tri;
 }
 
+tristate sym_get_minimal_value(struct symbol *sym)
+{
+	return sym->rev_dep.tri;
+}
+
+tristate sym_get_maximal_value(struct symbol *sym)
+{
+	return sym->visible;
+}
+
 struct symbol *sym_lookup(const char *name, int isconst)
 {
 	struct symbol *symbol;

1.5.1.6

Hi,

On i86_64 (but I think the arch does not matter).
make defconfig
make menuconfig

At top-level menu I see:
--- Enable the block layer --->

In block/Kconfig we have:
menuconfig BLOCK
bool "Enable the block layer" if EMBEDDED
default y

If EMBEDDED == n then we see the above.
And this was my first experience with this patch - and it took me some thoughts to realise it was the "if EMBEDDED" part that made it look like -*-

So maybe not something we see in many places but it is there in the top-level menu.

Sam

The v2 was maybe more intuitive, but had at least one flaw, where it claimed the option was selected by another, while it was in fact only made unchangeable by 'bool "Enable block layer" if EMBEDDED', defaulting to y.

Where exactly does this happen? There are cases when maximum < minimum, for example when you <M> THINKPAD_ACPI, then <N> BACKLIGHT_LCD_SUPPORT. After that, BACKLIGHT_CLASS_DEVICE has minimum of 1 (selected by THINKPAD_ACPI) and maximum 0 (depends on BACKLIGHT_LCD_SUPPORT, which is n)

The function names are maybe suboptimal, I agree.

Try saying <M> to MAC80211, wireless extensions were -*- with v2, now are (*) with v3. Also the symbols ( ), { } are changed and more well-defined in v3 IMHO.

Regards,
Matěj

This is easy to fix - example below.

Now a comment looks like this

*** This is a comment ***

Notice the three leading spaces which is where '---' was before.

Sam

index bc5854e..2ee12a7 100644
@@ -481,6 +481,14 @@ static void build_conf(struct menu *menu)
 				if (single_menu_mode && menu->data)
 					goto conf_childs;
 				return;
+			case P_COMMENT:
+				if (prompt) {
+					child_count++;
+					item_make("   %*c*** %s ***", indent + 1, ' ', prompt);
+					item_set_tag(':');
+					item_set_data(menu);
+				}
+				break;
 			default:
 				if (prompt) {
 					child_count++;

Definitely this is the best solution for the comment ambiguity.

So my patch turned out to solve only these questions:
"What does the '--- Wireless extensions' mean? Included or excluded?" "Why I cannot disable '<M> Option XXX'?"

Dunno if it is still worth merging. (but even if not, it at least made you solve the comments problem) :)

Matěj.

Hi,

The point is that I'm getting more concerned about overloading the interface with nontrivial information.
Another direction to consider would be to add this information to the help text, e.g. choose one syntax for nonchangable symbols and then the user can press help to find more detailed information.

sym_tristate_within_range() checks whether the new value is ok and sym_calc_value() uses rev_dep to override the value when necessary.

The actual maximum value is then 1.

The variable name is already correct, it's the visibility value of a symbol not its maximum. In the case of the "if EMBEDDED" then individual menu entries can still be visible, if any child entry is visible (see menu_is_visible()).

bye, Roman

Hi,

Indeed, I forgot about this case. Here it's actually the menu entry of a symbol that is forced to be visible, because a child entry is visible.

bye, Roman

If I understand clearly, something similar is already in v3 (hunk took from in-progress v4):
@@ -359,6 +369,11 @@ static void get_symbol_str(struct gstr *r, struct symbol *sym)

        str_printf(r, "Symbol: %s [=%s]\n", sym->name,
                                       sym_get_string_value(sym));

+ if (sym_get_rev_dep(sym) != no)
+ str_printf(r, "Enforced value: %s (see Selected by:)\n",
+ sym_get_rev_dep(sym) == mod ? "[m] or [y]" : "[y]");
+ if (sym_get_visibility(sym) == no)
+ str_append(r, _("None of the prompts active, default value assigned\n"));

        for_all_prompts(sym, prop)
                get_prompt_str(r, prop);

Changed function names to sym_get_rev_dep() and sym_get_visibility(). Shouldn't I move them from symbol.c and lkc_proto.h into lkc.h? They would fit into the section with static inline one-liners.

Bye,
Matej.

This little patch changes notation of unchangeable menuconfig items to always reflect the current state of inclusion of a feature.

---
Hi again, Sam, Roman, Randy.

Instead of incorporating all changes into a single patch, I decided to split things into more rather independent patches.

Expect another one extending show_help(). (which ATM needs more polishing before being posted)

 scripts/kconfig/mconf.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

index bc5854e..38ffbe1 100644
@@ -542,14 +542,19 @@ static void build_conf(struct menu *menu)
 			return;
 		}
 	} else {
+		val = sym_get_tristate_value(sym);
+		switch (val) {
+			case yes: ch = '*'; break;
+			case mod: ch = 'M'; break;
+			default:  ch = ' '; break;
+		}
 		if (menu == current_menu) {
-			item_make("---%*c%s", indent + 1, ' ', menu_get_prompt(menu));
+			item_make("-%c-%*c%s", ch, indent + 1, ' ', menu_get_prompt(menu));
 			item_set_tag(':');
 			item_set_data(menu);
 			goto conf_childs;
 		}
 		child_count++;
-		val = sym_get_tristate_value(sym);
 		if (sym_is_choice_value(sym) && val == yes) {
 			item_make("   ");
 			item_set_tag(':');

@@ -558,22 +563,17 @@ static void build_conf(struct menu *menu)
 			switch (type) {
 			case S_BOOLEAN:
 				if (sym_is_changable(sym))
-					item_make("[%c]", val == no ? ' ' : '*');
+					item_make("[%c]", ch);
 				else
-					item_make("---");
+					item_make("-%c-", ch);
 				item_set_tag('t');
 				item_set_data(menu);
 				break;
 			case S_TRISTATE:
-				switch (val) {
-				case yes: ch = '*'; break;
-				case mod: ch = 'M'; break;
-				default:  ch = ' '; break;
-				}
 				if (sym_is_changable(sym))
 					item_make("<%c>", ch);
 				else
-					item_make("---");
+					item_make("-%c-", ch);
 				item_set_tag('t');
 				item_set_data(menu);
 				break;

1.5.1.6