[PATCH] wrap long help lines, take two

[PATCH] wrap long help lines, take two

[Vadim Bendebury (вб)]

Help text for certain config options is very extensive (the text includes all
config options which the option in question depends on). Long lines are not
wrapped, making it impossible to see the list.

This patch adds some logic which wraps help screen lines at word
boundaries to prevent truncating .

Tested by running

mkdir ../build/powerpc
ARCH=powerpc make menuconfig O=../build/powerpc

in the kernel sorce tree. Once menu is displayed, hitting c<cr>cc? brings up
the help message for CONFIG_CRYPTO_MANAGER, now properly wrapped.

Signed-off-by: Vadim Bendebury <vbendeb@google.com>
---
 scripts/kconfig/expr.c |   20 ++++++++++++++++++--
 1 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/scripts/kconfig/expr.c b/scripts/kconfig/expr.c
index 2aa49e8..203eb6d 100644
--- a/scripts/kconfig/expr.c
+++ b/scripts/kconfig/expr.c
@@ -1099,9 +1099,25 @@ void expr_fprint(struct expr *e, FILE *out)

 static void expr_print_gstr_helper(void *data, struct symbol *sym,
const char *str)
 {
-       str_append((struct gstr*)data, str);
+       struct gstr *gs = (struct gstr*)data;
+       unsigned extra_length = strlen(str) + (sym ? 4 : 0);
+       const char *last_cr = strrchr(gs->s, '\n');
+       unsigned screen_width = getmaxx(stdscr) - 10;
+       unsigned last_line_length;
+
+       if (!last_cr) {
+               last_cr = gs->s;
+       }
+
+       last_line_length = strlen(gs->s) - (last_cr - gs->s);
+
+       if ((last_line_length + extra_length) > screen_width) {
+               str_append(gs, "\\\n");
+       }
+
+       str_append(gs, str);
        if (sym)
-               str_printf((struct gstr*)data, " [=%s]",
sym_get_string_value(sym));
+               str_printf(gs, " [=%s]", sym_get_string_value(sym));
 }

 void expr_gstr_print(struct expr *e, struct gstr *gs)
--
1.5.4.3

Re: [PATCH] wrap long help lines, take two

[Michal Marek]

On 15.12.2009 07:46, Vadim Bendebury (вб) wrote:
> Tested by running
> 
> mkdir ../build/powerpc
> ARCH=powerpc make menuconfig O=../build/powerpc

Did you test this exact patch? :) I'm getting
  HOSTCC  scripts/kconfig/zconf.tab.o
In file included from scripts/kconfig/zconf.tab.c:2452:
/home/mmarek/linux-2.6/scripts/kconfig/expr.c: In function
‘expr_print_gstr_helper’:
/home/mmarek/linux-2.6/scripts/kconfig/expr.c:1103: warning: implicit
declaration of function ‘getmaxx’
/home/mmarek/linux-2.6/scripts/kconfig/expr.c:1103: error: ‘stdscr’
undeclared (first use in this function)
/home/mmarek/linux-2.6/scripts/kconfig/expr.c:1103: error: (Each
undeclared identifier is reported only once
/home/mmarek/linux-2.6/scripts/kconfig/expr.c:1103: error: for each
function it appears in.)
make[2]: *** [scripts/kconfig/zconf.tab.o] Error 1

Anyway, it's better than the last version, but see comments below.

> diff --git a/scripts/kconfig/expr.c b/scripts/kconfig/expr.c
> index 2aa49e8..203eb6d 100644
> --- a/scripts/kconfig/expr.c
> +++ b/scripts/kconfig/expr.c
> @@ -1099,9 +1099,25 @@ void expr_fprint(struct expr *e, FILE *out)
> 
>  static void expr_print_gstr_helper(void *data, struct symbol *sym,
> const char *str)
>  {
> -       str_append((struct gstr*)data, str);
> +       struct gstr *gs = (struct gstr*)data;
> +       unsigned extra_length = strlen(str) + (sym ? 4 : 0);
> +       const char *last_cr = strrchr(gs->s, '\n');
> +       unsigned screen_width = getmaxx(stdscr) - 10;
> +       unsigned last_line_length;
> +
> +       if (!last_cr) {
> +               last_cr = gs->s;
> +       }
> +
> +       last_line_length = strlen(gs->s) - (last_cr - gs->s);

You can avoid recomputing all this if you store the state in a struct
that gets passed as the data argument:

struct somename {
  struct gstr *gstr;
  int width;
  /* other bookkeeping stuff like last_line_length */
}

Then expr_gstr_print would just initialize an instance of this structure
and pass it to expr_print as the callback argument. Also, computing the
desired width should be done in mconf and passed to expr_gstr_print as
an argument. First, the other config programs (conf, qconf and gconf)
should not depend on ncurses (they can pass a zero, meaning no
wrapping), second the getmaxx(stdscr) - 10 logic really belongs to
mconf. If we make use of this feature in nconf, then there the right
number be getmaxx(stdscr) - 12 or whatever, depending on the exact layout.

Michal

> +
> +       if ((last_line_length + extra_length) > screen_width) {
> +               str_append(gs, "\\\n");
> +       }
> +
> +       str_append(gs, str);
>         if (sym)
> -               str_printf((struct gstr*)data, " [=%s]",
> sym_get_string_value(sym));
> +               str_printf(gs, " [=%s]", sym_get_string_value(sym));
>  }
> 
>  void expr_gstr_print(struct expr *e, struct gstr *gs)
> --
> 1.5.4.3

Re: [PATCH] wrap long help lines, take two

[Vadim Bendebury (вб)]

2009/12/15 Michal Marek <mmarek@suse.cz>:
> On 15.12.2009 07:46, Vadim Bendebury (вб) wrote:
>> Tested by running
>>
>> mkdir ../build/powerpc
>> ARCH=powerpc make menuconfig O=../build/powerpc
>
> Did you test this exact patch? :) I'm getting
>  HOSTCC  scripts/kconfig/zconf.tab.o

That's embarrassing, I was wondering if it would compile and then
forgot to check why it does not complain.. I never did anything with
git before, this is and missed the fact that the previous change was
lingering in my tree. This is taken care of now.

> In file included from scripts/kconfig/zconf.tab.c:2452:
> /home/mmarek/linux-2.6/scripts/kconfig/expr.c: In function
> ‘expr_print_gstr_helper’:
> /home/mmarek/linux-2.6/scripts/kconfig/expr.c:1103: warning: implicit
> declaration of function ‘getmaxx’
> /home/mmarek/linux-2.6/scripts/kconfig/expr.c:1103: error: ‘stdscr’
> undeclared (first use in this function)
> /home/mmarek/linux-2.6/scripts/kconfig/expr.c:1103: error: (Each
> undeclared identifier is reported only once
> /home/mmarek/linux-2.6/scripts/kconfig/expr.c:1103: error: for each
> function it appears in.)
> make[2]: *** [scripts/kconfig/zconf.tab.o] Error 1
>
> Anyway, it's better than the last version, but see comments below.
>
>> diff --git a/scripts/kconfig/expr.c b/scripts/kconfig/expr.c
>> index 2aa49e8..203eb6d 100644
>> --- a/scripts/kconfig/expr.c
>> +++ b/scripts/kconfig/expr.c
>> @@ -1099,9 +1099,25 @@ void expr_fprint(struct expr *e, FILE *out)
>>
>>  static void expr_print_gstr_helper(void *data, struct symbol *sym,
>> const char *str)
>>  {
>> -       str_append((struct gstr*)data, str);
>> +       struct gstr *gs = (struct gstr*)data;
>> +       unsigned extra_length = strlen(str) + (sym ? 4 : 0);
>> +       const char *last_cr = strrchr(gs->s, '\n');
>> +       unsigned screen_width = getmaxx(stdscr) - 10;
>> +       unsigned last_line_length;
>> +
>> +       if (!last_cr) {
>> +               last_cr = gs->s;
>> +       }
>> +
>> +       last_line_length = strlen(gs->s) - (last_cr - gs->s);
>
> You can avoid recomputing all this if you store the state in a struct
> that gets passed as the data argument:
>
> struct somename {
>  struct gstr *gstr;
>  int width;
>  /* other bookkeeping stuff like last_line_length */
> }
>
> Then expr_gstr_print would just initialize an instance of this structure
> and pass it to expr_print as the callback argument.

I first thought about it, but decided against it, because the way it
is now it is a much more contained change. Yes, recalculating string
length every time is excessive, but in this particular case it is
harmless.I am also worried of the string modified elsewhere bypassing
the print helper function, which would take things out of sync and
cause corrupted data.

 Please take another look at this, I can change it, but I think a
smaller modification has merits.

> Also, computing the
> desired width should be done in mconf and passed to expr_gstr_print as
> an argument. First, the other config programs (conf, qconf and gconf)
> should not depend on ncurses (they can pass a zero, meaning no
> wrapping), second the getmaxx(stdscr) - 10 logic really belongs to
> mconf. If we make use of this feature in nconf, then there the right
> number be getmaxx(stdscr) - 12 or whatever, depending on the exact layout.
>
good point, I'll take care of this.

cheers,
/vb


> Michal
>
>> +
>> +       if ((last_line_length + extra_length) > screen_width) {
>> +               str_append(gs, "\\\n");
>> +       }
>> +
>> +       str_append(gs, str);
>>         if (sym)
>> -               str_printf((struct gstr*)data, " [=%s]",
>> sym_get_string_value(sym));
>> +               str_printf(gs, " [=%s]", sym_get_string_value(sym));
>>  }
>>
>>  void expr_gstr_print(struct expr *e, struct gstr *gs)
>> --
>> 1.5.4.3
>
>

Re: [PATCH] wrap long help lines, take two

[Michal Marek]

On 16.12.2009 05:56, Vadim Bendebury (вб) wrote:
> 2009/12/15 Michal Marek <mmarek@suse.cz>:
>> struct somename {
>>  struct gstr *gstr;
>>  int width;
>>  /* other bookkeeping stuff like last_line_length */
>> }
>>
>> Then expr_gstr_print would just initialize an instance of this structure
>> and pass it to expr_print as the callback argument.
> 
> I first thought about it, but decided against it, because the way it
> is now it is a much more contained change. Yes, recalculating string
> length every time is excessive, but in this particular case it is
> harmless.I am also worried of the string modified elsewhere bypassing
> the print helper function, which would take things out of sync and
> cause corrupted data.

expr_print() only sees a void pointer, so it can't modify any data
hidden behind it. But I don't care, do it as you like, just please move
the ncurses calls to menuconfig.

Michal

Re: [PATCH] wrap long help lines, take two

[Vadim Bendebury (вб)]

2009/12/16 Michal Marek <mmarek@suse.cz>
>
> On 16.12.2009 05:56, Vadim Bendebury (вб) wrote:
> > 2009/12/15 Michal Marek <mmarek@suse.cz>:
> >> struct somename {
> >>  struct gstr *gstr;
> >>  int width;
> >>  /* other bookkeeping stuff like last_line_length */
> >> }
> >>
> >> Then expr_gstr_print would just initialize an instance of this structure
> >> and pass it to expr_print as the callback argument.
> >
> > I first thought about it, but decided against it, because the way it
> > is now it is a much more contained change. Yes, recalculating string
> > length every time is excessive, but in this particular case it is
> > harmless.I am also worried of the string modified elsewhere bypassing
> > the print helper function, which would take things out of sync and
> > cause corrupted data.
>
> expr_print() only sees a void pointer, so it can't modify any data
> hidden behind it. But I don't care, do it as you like, just please move
> the ncurses calls to menuconfig.
>
> Michal

Michal I looked at it a bit more, it seems that passing the width as
an argument to expr_gstr_print() is not such a good idea, especially
since it needs to originate in mconf.c:show_help() and make it all the
way to expr.c:expr_print_gstr_helper() - it's quite a few nested calls
where the argument needs to propagate through.

I ended up modifying the gstr structure adding a field to specify the
max width. It is excessive for other than mconf modes, but it seems a
small price to pay  to get a smallest contained change.

Please let me know what you think and if I need to resend the patch in
a separate email:

---

Help text for certain config options is very extensive (the text
includes the names of
all  other options the option in question depends on). Long lines are
not wrapped,
making it impossible to see the list.

This patch adds some logic which wraps help screen lines at word
boundaries to prevent truncating .

Tested by running

ARCH=powerpc make menuconfig O=/tmp/build

which shows that the long lines are now wrapped, and

 ARCH=powerpc make xconfig O=/tmp/build

to demonstrate that it still compiles and operates as expected.

Signed-off-by: Vadim Bendebury <vbendeb@google.com>
---
diff --git a/scripts/kconfig/expr.c b/scripts/kconfig/expr.c
index edd3f39..d83f232 100644
--- a/scripts/kconfig/expr.c
+++ b/scripts/kconfig/expr.c
@@ -1097,9 +1097,32 @@ void expr_fprint(struct expr *e, FILE *out)

 static void expr_print_gstr_helper(void *data, struct symbol *sym,
const char *str)
 {
-       str_append((struct gstr*)data, str);
+       struct gstr *gs = (struct gstr*)data;
+       const char *sym_str = NULL;
+
+       if (sym)
+               sym_str = sym_get_string_value(sym);
+
+       if (gs->max_width) {
+               unsigned extra_length = strlen(str);
+               const char *last_cr = strrchr(gs->s, '\n');
+               unsigned last_line_length;
+
+               if (sym_str)
+                       extra_length += 4 + strlen(sym_str);
+
+               if (!last_cr)
+                       last_cr = gs->s;
+
+               last_line_length = strlen(gs->s) - (last_cr - gs->s);
+
+               if ((last_line_length + extra_length) > gs->max_width)
+                       str_append(gs, "\\\n");
+       }
+
+       str_append(gs, str);
        if (sym)
-               str_printf((struct gstr*)data, " [=%s]",
sym_get_string_value(sym));
+               str_printf(gs, " [=%s]", sym_str);
 }

 void expr_gstr_print(struct expr *e, struct gstr *gs)
diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h
index f379b0b..95ce009 100644
--- a/scripts/kconfig/lkc.h
+++ b/scripts/kconfig/lkc.h
@@ -106,6 +106,11 @@ int file_write_dep(const char *name);
 struct gstr {
        size_t len;
        char  *s;
+       /*
+        * when max_width is not zero long lines in string s (if any) get
+        * wrapped not to exceed the max_width value
+        */
+       int max_width;
 };
 struct gstr str_new(void);
 struct gstr str_assign(const char *s);
diff --git a/scripts/kconfig/mconf.c b/scripts/kconfig/mconf.c
index 8413cf3..ac1e9da 100644
--- a/scripts/kconfig/mconf.c
+++ b/scripts/kconfig/mconf.c
@@ -638,6 +638,7 @@ static void show_help(struct menu *menu)
 {
        struct gstr help = str_new();

+       help.max_width = getmaxx(stdscr) - 10;
        menu_get_ext_help(menu, &help);

        show_helptext(_(menu_get_prompt(menu)), str_get(&help));
diff --git a/scripts/kconfig/util.c b/scripts/kconfig/util.c
index b6b2a46..81c100d 100644
--- a/scripts/kconfig/util.c
+++ b/scripts/kconfig/util.c
@@ -78,6 +78,7 @@ struct gstr str_new(void)
        struct gstr gs;
        gs.s = malloc(sizeof(char) * 64);
        gs.len = 64;
+       gs.max_width = 0;
        strcpy(gs.s, "\0");
        return gs;
 }
@@ -88,6 +89,7 @@ struct gstr str_assign(const char *s)
        struct gstr gs;
        gs.s = strdup(s);
        gs.len = strlen(s) + 1;
+       gs.max_width = 0;
        return gs;
 }

---

Re: [PATCH] wrap long help lines, take two

[Michal Marek]

Vadim Bendebury (вб) napsal(a):
> Michal I looked at it a bit more, it seems that passing the width as
> an argument to expr_gstr_print() is not such a good idea, especially
> since it needs to originate in mconf.c:show_help() and make it all the
> way to expr.c:expr_print_gstr_helper() - it's quite a few nested calls
> where the argument needs to propagate through.
> 
> I ended up modifying the gstr structure adding a field to specify the
> max width. It is excessive for other than mconf modes, but it seems a
> small price to pay  to get a smallest contained change.
> 
> Please let me know what you think and if I need to resend the patch in
> a separate email:

Hi Vadim,

sorry for the delay. Yes this patch looks ok, I added it to the kbuild tree.

Thanks!
Michal