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.
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
