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

[Sascha Hauer <s.hauer@pengutronix.de>]

On Thu, Aug 18, 2016 at 12:36:29PM +0300, Aleksey Kuleshov wrote:
> >>  @@ -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;
> +       }

Ok, looks good.

> > 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.

Yes, strike! ;)

> >>
> >>  - 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 */

You can use strim for both cases:

	str = strim(str);

removes both leading and trailing whitespaces, but without reassigning
str to the return value of strim() you only remove trailing whitespaces.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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