[PATCH] normal: fix get_logical_num_lines

[PATCH] normal: fix get_logical_num_lines

[Michael Chang]

In menu editing mode, grub2 shows bogus line if the character being
edited is at last column of entry. This patch fixes the problem by
having the get_logical_num_lines function to calculate correct number of
lines.

---
 grub-core/normal/menu_entry.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grub-core/normal/menu_entry.c b/grub-core/normal/menu_entry.c
index 62c7e16..1d4b0c6 100644
--- a/grub-core/normal/menu_entry.c
+++ b/grub-core/normal/menu_entry.c
@@ -128,7 +128,7 @@ get_logical_num_lines (struct line *linep, struct per_term_screen *term_screen)
 {
   return (grub_getstringwidth (linep->buf, linep->buf + linep->len,
 			       term_screen->term)
-	  / (unsigned) term_screen->geo.entry_width) + 1;
+	  / ((unsigned) term_screen->geo.entry_width + 1)) + 1;
 }
 
 static void
-- 
2.6.3

Re: [PATCH] normal: fix get_logical_num_lines

[Andrei Borzenkov]

On Wed, Dec 23, 2015 at 7:45 AM, Michael Chang <mchang@suse.com> wrote:
> In menu editing mode, grub2 shows bogus line if the character being
> edited is at last column of entry. This patch fixes the problem by
> having the get_logical_num_lines function to calculate correct number of
> lines.
>

There is one more occurrence in update_screen (); could you check if
this needs fix too?

  grub_size_t t = grub_getstringwidth (linep->buf, linep->buf + screen->column,
      term_screen->term);
  y += t / (unsigned) term_screen->geo.entry_width;


> ---
>  grub-core/normal/menu_entry.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/grub-core/normal/menu_entry.c b/grub-core/normal/menu_entry.c
> index 62c7e16..1d4b0c6 100644
> --- a/grub-core/normal/menu_entry.c
> +++ b/grub-core/normal/menu_entry.c
> @@ -128,7 +128,7 @@ get_logical_num_lines (struct line *linep, struct per_term_screen *term_screen)
>  {
>    return (grub_getstringwidth (linep->buf, linep->buf + linep->len,
>                                term_screen->term)
> -         / (unsigned) term_screen->geo.entry_width) + 1;
> +         / ((unsigned) term_screen->geo.entry_width + 1)) + 1;
>  }
>
>  static void
> --
> 2.6.3
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] normal: fix get_logical_num_lines

[Michael Chang]

On Wed, Dec 23, 2015 at 09:26:43AM +0300, Andrei Borzenkov wrote:
> On Wed, Dec 23, 2015 at 7:45 AM, Michael Chang <mchang@suse.com> wrote:
> > In menu editing mode, grub2 shows bogus line if the character being
> > edited is at last column of entry. This patch fixes the problem by
> > having the get_logical_num_lines function to calculate correct number of
> > lines.
> >
> 
> There is one more occurrence in update_screen (); could you check if
> this needs fix too?
> 
>   grub_size_t t = grub_getstringwidth (linep->buf, linep->buf + screen->column,
>       term_screen->term);
>   y += t / (unsigned) term_screen->geo.entry_width;

No, as the check followed has bailed it out.

  if (t % (unsigned) term_screen->geo.entry_width == 0
        && t != 0 &&  screen->column == linep->len)
	    y--;

Of course, we can try to make the code more terse by the same fix so
that we can remove the check. Should I submit a new patch for that ? 

Thanks,
Michael

Re: [PATCH] normal: fix get_logical_num_lines

[Andrei Borzenkov]

On Wed, Dec 23, 2015 at 7:45 AM, Michael Chang <mchang@suse.com> wrote:
> In menu editing mode, grub2 shows bogus line if the character being
> edited is at last column of entry. This patch fixes the problem by
> having the get_logical_num_lines function to calculate correct number of
> lines.
>
> ---
>  grub-core/normal/menu_entry.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/grub-core/normal/menu_entry.c b/grub-core/normal/menu_entry.c
> index 62c7e16..1d4b0c6 100644
> --- a/grub-core/normal/menu_entry.c
> +++ b/grub-core/normal/menu_entry.c
> @@ -128,7 +128,7 @@ get_logical_num_lines (struct line *linep, struct per_term_screen *term_screen)
>  {
>    return (grub_getstringwidth (linep->buf, linep->buf + linep->len,
>                                term_screen->term)
> -         / (unsigned) term_screen->geo.entry_width) + 1;
> +         / ((unsigned) term_screen->geo.entry_width + 1)) + 1;

No, that's wrong. Consider entry_width = 10 and grub_getstringwidth =
21. It needs 3 lines but your change gives only 2.

It sounds like we need

string_width = grub_getstringwidth (linep->buf, linep->buf +
linep->len, term_screen->term);
if (!string_width)
  return 1;
return (string_width + (unsigned) term_screen->geo.entry_width - 1) /
(unsigned) term_screen->geo.entry_width;

Could you test if it works for you?

Re: [PATCH] normal: fix get_logical_num_lines

[Michael Chang]

On Thu, Dec 24, 2015 at 02:48:34PM +0300, Andrei Borzenkov wrote:
> On Wed, Dec 23, 2015 at 7:45 AM, Michael Chang <mchang@suse.com> wrote:
> > In menu editing mode, grub2 shows bogus line if the character being
> > edited is at last column of entry. This patch fixes the problem by
> > having the get_logical_num_lines function to calculate correct number of
> > lines.
> >
> > ---
> >  grub-core/normal/menu_entry.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/grub-core/normal/menu_entry.c b/grub-core/normal/menu_entry.c
> > index 62c7e16..1d4b0c6 100644
> > --- a/grub-core/normal/menu_entry.c
> > +++ b/grub-core/normal/menu_entry.c
> > @@ -128,7 +128,7 @@ get_logical_num_lines (struct line *linep, struct per_term_screen *term_screen)
> >  {
> >    return (grub_getstringwidth (linep->buf, linep->buf + linep->len,
> >                                term_screen->term)
> > -         / (unsigned) term_screen->geo.entry_width) + 1;
> > +         / ((unsigned) term_screen->geo.entry_width + 1)) + 1;
> 
> No, that's wrong. Consider entry_width = 10 and grub_getstringwidth =
> 21. It needs 3 lines but your change gives only 2.

Alas! Indeed I am mistaken here. We should minus the numerator by one
but not adding one to denominator. Thanks for your check. :)

> 
> It sounds like we need
> 
> string_width = grub_getstringwidth (linep->buf, linep->buf +
> linep->len, term_screen->term);
> if (!string_width)
>   return 1;
> return (string_width + (unsigned) term_screen->geo.entry_width - 1) /
> (unsigned) term_screen->geo.entry_width;
> 
> Could you test if it works for you?

Yes. It works great.

Thanks,
Michael

Re: [PATCH] normal: fix get_logical_num_lines

[Michael Chang]

On Mon, Dec 28, 2015 at 12:09:58PM +0800, Michael Chang wrote:
> On Thu, Dec 24, 2015 at 02:48:34PM +0300, Andrei Borzenkov wrote:
> > It sounds like we need
> > 
> > string_width = grub_getstringwidth (linep->buf, linep->buf +
> > linep->len, term_screen->term);
> > if (!string_width)
> >   return 1;
> > return (string_width + (unsigned) term_screen->geo.entry_width - 1) /
> > (unsigned) term_screen->geo.entry_width;
> > 
> > Could you test if it works for you?
> 
> Yes. It works great.

Hi Andrei,

I found we had a down stream bug opened for the problem a while ago that
could be fixed by the patch.

https://bugzilla.suse.com/show_bug.cgi?id=943585

So please consider to commit your patch if you think it's ready to go as
well after the testing.

Thanks,
Michael

Re: [PATCH] normal: fix get_logical_num_lines

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1969 bytes --]

On 28.12.2015 05:09, Michael Chang wrote:
> On Thu, Dec 24, 2015 at 02:48:34PM +0300, Andrei Borzenkov wrote:
>> On Wed, Dec 23, 2015 at 7:45 AM, Michael Chang <mchang@suse.com> wrote:
>>> In menu editing mode, grub2 shows bogus line if the character being
>>> edited is at last column of entry. This patch fixes the problem by
>>> having the get_logical_num_lines function to calculate correct number of
>>> lines.
>>>
>>> ---
>>>  grub-core/normal/menu_entry.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/grub-core/normal/menu_entry.c b/grub-core/normal/menu_entry.c
>>> index 62c7e16..1d4b0c6 100644
>>> --- a/grub-core/normal/menu_entry.c
>>> +++ b/grub-core/normal/menu_entry.c
>>> @@ -128,7 +128,7 @@ get_logical_num_lines (struct line *linep, struct per_term_screen *term_screen)
>>>  {
>>>    return (grub_getstringwidth (linep->buf, linep->buf + linep->len,
>>>                                term_screen->term)
>>> -         / (unsigned) term_screen->geo.entry_width) + 1;
>>> +         / ((unsigned) term_screen->geo.entry_width + 1)) + 1;
>>
>> No, that's wrong. Consider entry_width = 10 and grub_getstringwidth =
>> 21. It needs 3 lines but your change gives only 2.
> 
> Alas! Indeed I am mistaken here. We should minus the numerator by one
> but not adding one to denominator. Thanks for your check. :)
> 
>>
>> It sounds like we need
>>
>> string_width = grub_getstringwidth (linep->buf, linep->buf +
>> linep->len, term_screen->term);
>> if (!string_width)
>>   return 1;
>> return (string_width + (unsigned) term_screen->geo.entry_width - 1) /
>> (unsigned) term_screen->geo.entry_width;
>>
>> Could you test if it works for you?
> 
> Yes. It works great.
@Anrei: please go ahead.
> 
> Thanks,
> Michael
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]