Re: [PATCH 2/2] rework menu so that it can support multiline titles

[Aleksey Kuleshov <rndfax@yandex.ru>]

>>  @@ -234,12 +237,19 @@ static void print_menu(struct menu *m)
>>           struct menu_entry *me;
>>
>>           clear();
>>  - gotoXY(2, 1);
>>  - if(m->display) {
>>  - __print_entry(m->display);
>>  + if (m->display) {
>>  + int i;
>>  + for (i = 0; i < m->display_lines; i++) {
>>  + gotoXY(2, 1 + i);
>>  + __print_entry(m->display[i]);
>>  + }
>>  + m->skip_lines = 0;
>>           } else {
>>  + gotoXY(2, 1);
>>                   puts("Menu : ");
>>  - puts(m->name);
>>  + if (m->name)
>>  + puts(m->name);
>>  + m->skip_lines = 1;
>>           }
>
> We could add this to menu_add():
>
>        if (!m->display)
>                m->display = xasprintf("Menu : %s", m->name);
>
> Then we wouldn't need the if(m->display) here. Would that simplify the
> code?

Yes. But all title things are go into menu_add_titile, so how about this:

diff --git a/common/menu.c b/common/menu.c
index 1f23e45..636c2b8 100644
--- a/common/menu.c
+++ b/common/menu.c
@@ -235,21 +235,12 @@ EXPORT_SYMBOL(menu_set_auto_select);
 static void print_menu(struct menu *m)
 {
        struct menu_entry *me;
+       int i;

        clear();
-       if (m->display) {
-               int i;
-               for (i = 0; i < m->display_lines; i++) {
-                       gotoXY(2, 1 + i);
-                       __print_entry(m->display[i]);
-               }
-               m->skip_lines = 0;
-       } else {
-               gotoXY(2, 1);
-               puts("Menu : ");
-               if (m->name)
-                       puts(m->name);
-               m->skip_lines = 1;
+       for (i = 0; i < m->display_lines; i++) {
+               gotoXY(2, 1 + i);
+               __print_entry(m->display[i]);
        }

        list_for_each_entry(me, &m->entries, list) {
@@ -538,8 +529,12 @@ void menu_add_title(struct menu *m, const char *display, char *(*parser)(char *s
        int lines = 1;
        int i;

-       if (!strlen(display))
+       if (!strlen(display)) {
+               m->display_lines = 1;
+               m->display = xzalloc(sizeof(*m->display));
+               m->display[0] = xasprintf("Menu : %s", m->name ? m->name : "");
                return;
+       }

        src = dst = tmp = xstrdup(display);
        /* Count lines and separate single string into multiple strings */

+ remove skip_lines everywhere.

> I haven't really understood why the m->skip_lines is necessary, but I
> think it should be removed by making the if/else above unnecessary.

For proper spacing. But yes, it can be removed with patch above.

>>  + for (src = tmp, i = 0; i < lines; i++) {
>>  + if (parser)
>>  + m->display[i] = parser(src);
>>  + else
>>  + m->display[i] = xstrdup(src);
>
> What's the reason for running parser() over the separated strings? Can't
> you run parser() over the original multiline string instead?

I wanted to consume more CPU cycles so that the Earth will suffer from power starvation. But you ruined my evil plan, clap-clap-clap.

> What if one of the variables expanded in shell_expand() contains a newline
> character?

It will be incorrect rendering. C u in patch v2 series.

>
>>  + /* Go to the next line */
>>  + src += strlen(src) + 1;
>>  + }
>>  +
>>  + free(tmp);
>>  +}
>>  +EXPORT_SYMBOL(menu_add_title);
>>  diff --git a/common/menutree.c b/common/menutree.c
>>  index eb14da0..e2d364d 100644
>>  --- a/common/menutree.c
>>  +++ b/common/menutree.c
>>  @@ -19,6 +19,7 @@
>>   #include <shell.h>
>>   #include <libfile.h>
>>
>>  +#include <linux/ctype.h>
>>   #include <linux/stat.h>
>>
>>   struct menutree {
>>  @@ -95,6 +96,7 @@ int menutree(const char *path, int toplevel)
>>           glob_t g;
>>           int i;
>>           char *globpath, *display;
>>  + size_t size;
>>
>>           menu = menu_alloc();
>>
>>  @@ -106,14 +108,21 @@ int menutree(const char *path, int toplevel)
>>                   goto out;
>>           }
>>
>>  - display = read_file_line("%s/title", path);
>>  + globpath = basprintf("%s/title", path);
>>  + display = read_file(globpath, &size);
>>  + /* Remove trailing whitespaces */
>>  + while (size > 0 && isspace(display[size - 1]))
>>  + size--;
>
> We have the strim() function for this purpose.

 * strim - Removes leading and trailing whitespace from @s.

And mine does only:
>>  + /* Remove trailing whitespaces */

Because it's necessary to preserve identation in multiline title so that barebox will be able to display such text:
Barebox boot loader
Board: SomeSuperBoard
    CPU: ARMv8 @800MHz
    Mem: 512MB

Menu:
    ....

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox