Re: Fwd: [PATCH] gfxpayload

[Robert Millan <rmh@aybabtu.com>]

Hi,

On Sun, May 17, 2009 at 04:34:16PM +0200, Vladimir 'phcoder' Serbinenko wrote:
>  #ifndef GRUB_ASSUME_LINUX_HAS_FB_SUPPORT
> -#define GRUB_ASSUME_LINUX_HAS_FB_SUPPORT 0
> +#define DEFAULT_VIDEO_MODE "text"
> +#else
> +#define DEFAULT_VIDEO_MODE "keep"
>  #endif

As discussed on IRC, this "keep" option seems unnecessary;  it just means
"do nothing" so we may as well #ifdef the code out.

> +	return grub_error (GRUB_ERR_OUT_OF_MEMORY, 
> +			   "couldn't allocate temporary storag");

Typo.

>  	if (grub_strcmp (val, "normal") == 0)
> -	  vid_mode = GRUB_LINUX_VID_MODE_NORMAL;
> +	  vid_mode = 0;
>  	else if (grub_strcmp (val, "ext") == 0)
> -	  vid_mode = GRUB_LINUX_VID_MODE_EXTENDED;
> +	  vid_mode = 1;
> [...]
>  	switch (vid_mode)
>  	  {
>  	  case 0:
> [...]
>  	  case 1:

Can we avoid encoding this information as numbers?  We had nice macros
before, and plain numbers are confusing to read for those not familiar with
the code.

> +	    grub_env_set ("gfxpayload", "text");
> +	    grub_printf ("%s is depreceated. "

Typo ;-)

> +	    buf = grub_malloc (20);
> +	    if (! buf)
> +	      {
> +		grub_error (GRUB_ERR_OUT_OF_MEMORY, 
> +			    "couldn't allocate temporary storage");
> +		goto fail;
> +	      }

Doesn't grub_malloc set this errno / errmsg already?

> --- a/term/gfxterm.c
> +++ b/term/gfxterm.c
> @@ -27,10 +27,7 @@
>  #include <grub/bitmap.h>
>  #include <grub/command.h>
>  
> -#define DEFAULT_VIDEO_WIDTH	640
> -#define DEFAULT_VIDEO_HEIGHT	480
> -#define DEFAULT_VIDEO_FLAGS	0
> -
> +#define DEFAULT_VIDEO_MODE "1024x768,800x600,640x480"

Our fonts / texts / menus in general are not prepared to cope well with
resolutions higher than 640x480.  It works, but looks different/ugly.
Please leave that as default untill we solve those problems.

Wrt changes in the video subsystem, as discussed on IRC this was discussed
before (me and Vesa, I think) and Vesa said he wanted to think about it.
Would be nice if we can reach consensus on this, and someone who's
familiarised with that area (not me) could approve those changes.

But Vesa is on vacation... maybe Colin?  I don't know.  Try CCing them :-)

Thanks!

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."