Re: [PATCH] GSoC #10 new font engine (vs r1885)

["Vesa Jääskeläinen" <chaac@nic.fi>]

Colin D Bennett wrote:
> Clean patch against SVN revision 1885.
> 
> Regards,
> Colin

Thanks for the re-base.

Here are some comments about the patch.

Check the commenting of multi line comments.

> +int
> +grub_font_get_string_width (grub_font_t font, const char *str)
> +{
> +  int i;
> +  int width;
> +  struct grub_font_glyph *glyph;
> +  grub_size_t len;
> +
> +  len = grub_strlen (str);
> +
> +  for (i = 0, width = 0; i < len; i++)
> +    {
> +      glyph = grub_font_get_glyph (font, str[i]);
> +      width += glyph->device_width;
> +    }
> +
> +  return width;
> +}

I do not see this being utf-8 compatible eg. it breaks our Unicode
displaying support. See how grub_putchar() handles this.

There is also the same problem in grub_video_vbe_draw_string().

> === modified file 'conf/sparc64-ieee1275.rmk'
> --- conf/sparc64-ieee1275.rmk	2008-09-08 12:52:30 +0000
> +++ conf/sparc64-ieee1275.rmk	2008-10-05 04:30:04 +0000

As font code is common code. There is no need to modify sparc64 specific
makefile. You have correctly modified common.rmk so that is enough. If
sparc64 support should be moved to use common.rmk like any other arch.
Let it rot.

> === added file 'font/loader.c'
> --- font/loader.c	1970-01-01 00:00:00 +0000
> +++ font/loader.c	2008-10-05 04:30:04 +0000

> +/*
> +   Read a 16-bit big-endian integer from FILE, convert it to native byte
> +   order, and store it in *VALUE.
> +   Returns 0 on success, 1 on failure.
> + */
> +static int
> +read_be_uint16 (grub_file_t file, grub_uint16_t * value)

Are you fan of bigendian or why did you choose that :) ?

> +struct grub_font_glyph *
> +grub_font_get_glyph (grub_font_t font, grub_uint32_t code)

I would propose to but all public API's to one file.

> === modified file 'include/grub/video.h'
> --- include/grub/video.h	2008-10-05 04:28:39 +0000
> +++ include/grub/video.h	2008-10-05 04:30:04 +0000

Now that you have moved glyph rendering out from specialized glyph
rendering to bitmap rendering I would propose that no font rendering
code resides in Video API. I think this is a good move.

I would move that to font.mod (or perhaps to graphical terminal?). We
can adapt other graphical terminals to use video API to render stuff. It
just takes a bit time. But I think this time is as good as any to start
that process.

Lets have another look at it after those modifications :)

Thanks,
Vesa Jääskeläinen