Re: [PATCH] background_color command

[Vladimir 'φ-coder/phcoder' Serbinenko]

On 12/10/2010 06:53 PM, Colin Watson wrote:
>
>  module = {
> +  name = video_colors;
> +  common = video/colors.c;
> +  enable = videomodules;
> +};
>   
Both main users of video (gfxmenu and gfxterm) use color routines. So I
feel like video/colors.c can go into video.mod

>
> +  /* Create a filled bitmap so that we get suitable text blending.  */
> +  grub_video_bitmap_create (&bitmap, window.width, window.height,
> +                            GRUB_VIDEO_BLIT_FORMAT_RGB_888);
> +  if (grub_errno != GRUB_ERR_NONE)
> +    return grub_errno;
>   
You should check the return value and not grub_errno.
> +
>   
It would be more optimal if in this case the image would be saved simply
as the color rather than a filled array and use grub_video_fill_rect
when this image is supposed to be blitted. also it would be neat to be
able to specify the bgcolor around non-stretched image.
> +  data = bitmap->data;
> +  for (i = 0; i < window.height * window.width; i++)
> +    {
> +      *data++ = color.red;
> +      *data++ = color.green;
> +      *data++ = color.blue;
> +    }
> +
>   
grub_video_fill_rect is the function for such kind of operations.
> +  bitmap_width = window.width;
> +  bitmap_height = window.height;
> +
> +  /* Set the border color.  */
> +  virtual_screen.bg_color_display = grub_video_map_rgba_color (color);
> +
> +  /* Mark whole screen as dirty.  */
> +  dirty_region_add (0, 0, window.width, window.height);
> +
> +  /* All was ok.  */
> +  grub_errno = GRUB_ERR_NONE;
>   
You shouldn't discard grub_errno manually unless there is an error you
want to ignore.
> +  return grub_errno;
>   
I'd rather use return GRUB_ERR_NONE;

-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko

signature.asc
Description: OpenPGP digital signature