Re: [PATCH] gfxpayload

[Colin D Bennett]

Robert Millan wrote on Sunday 17 May 2009:
> Hi,
>
> On Sun, May 17, 2009 at 04:34:16PM +0200, Vladimir 'phcoder' Serbinenko 
wrote:
> > --- 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.

I think the main problems at present are (1) the only provided font is GNU 
Unifont, which is fairly small (but I think it should work ok at sizes around 
1024x768) and (2) my graphical menu themes currently support only absolute 
pixel positioning, so when the screen size is changed, they don't adapt (I 
really want to implement some sort of layout management system to handle 
component layout consistently in the GUI).

> 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 :-)

I think your grub_video_set_mode() is a good change.  In fact, I also moved 
that mode selection code out of gfxterm and into the video API in my graphical 
menu branch.

It seems to me that there aren't significant changes to the video subsystem 
except that grub_video_setmode(width, height, mode_type) is removed and 
replaced with a function that takes only a mode list string.  I think this is 
fine since we should (once we get the above-mentioned issues resolved to 
support various resolutions properly) never hard-code video mode info (except 
for the 'videotest' command, perhaps; rather it should always be sourced from 
the user configuration, which means it will be a mode list string anyway.

I had a minor comment regarding the patch:

--- a/video/video.c
+++ b/video/video.c
...
+grub_err_t
+grub_video_set_mode (char *modestring,
+                    int NESTED_FUNC_ATTR (*hook) (grub_video_adapter_t p,
+                                                  struct grub_video_mode_info 
*mode_info))
+{
+  char *tmp;
+  char *next_mode;
+  char *current_mode;
+  char *param;
+  char *value;
+  char *modevar;
+  int width = -1;
+  int height = -1;
+  int depth = -1;
+  int flags = 0;
+
+  /* Take copy of env.var. as we don't want to modify that.  */
+  modevar = grub_strdup (modestring);

Since this is called in cases where 'modestring' is not an environment 
variable, maybe it's more correct to just say, "Copy the mode list argument 
because it will be modified."

I would declare modestring as 'const char *' to prevent accidental
modification, especially since there is code that passes string constants for
modestring.

Regards,
Colin

signature.asc
Description: This is a digitally signed message part.