Re: [RP] Xft patch

[Shawn Betts]

On Fri, May 23, 2008 at 3:59 AM, Midare Kiyura <address@hidden> wrote:
> [PATCH] Added Xft font support, enabled at configure time using 
> --with-xft[="Default Font"]. Large changes to the code were made, most 
> notably that all XmbTextEscapement and XmbDrawString calls were replaced with 
> wrapper functions, and the FONT_HEIGHT macro was redone. I used the patch 
> found here as a reference: 
> http://osdir.com/ml/window-managers.ratpoison.devel/2003-04/msg00111.html

This patch looks mostly good but I have some requests. Thanks to
Joshua for the original patch.
> +AC_ARG_WITH(xft,
> +       AC_HELP_STRING([--with-xft=FONT], [use FONT via the Xft library]),
> +       [if test "x$withval" != "xno"; then
> +                dnl Set a default font
> +               if test "x$withval" == "xyes"; then
> +                       withval="Mono-11"
> +               fi
> +               AC_DEFINE_UNQUOTED(USE_XFT_FONT, "$withval", [If defined: the 
> desired font])
> +               CFLAGS="$CFLAGS `pkg-config --cflags xft`"
> +               LDFLAGS="$LDFLAGS `pkg-config --libs xft`"
> +               AC_MSG_RESULT(yes)
> +       else
> +               AC_MSG_RESULT(no)
> +       fi],
> +       [AC_MSG_RESULT(no)])
> +

Would it be possible to simply turn on xft support and supply the
default font in conf.h as is currently done? Is it possible with this
patch to change the font at run time with "set font" if not I think
this needs to be added. With that in place setting the font in
configure becomes unneeded, assuming there is a default font that will
work everywhere.

> +             RPDrawString (s, wins[i], s->normal_gc,
> +                           defaults.bar_x_padding,
> +                           defaults.bar_y_padding + FONT_ASCENT(s),
> +                           num, -1);

Since rpdrawstring is part of ratpoison can you unstudly caps it to:
rp_draw_string?

> +      tmp = RPTextWidth (s, defaults.font, license_text[i], -1);

Same here.


> +void RPDrawString (rp_screen *s, Drawable d, GC gc, int x, int y, char 
> *string, int length);
> +int RPTextWidth (rp_screen *s, XFontSet font, char *string, int count);

Seems like globals.c/h is a better place for these two functions.

> --- a/src/screen.c
> +++ b/src/screen.c
> @@ -239,6 +239,9 @@ init_screen (rp_screen *s, int screen_num)
>  {
>   XGCValues gv;
>   int xine_screen_num;
> +#ifdef USE_XFT_FONT
> +  XRenderColor rc = {0, 0, 0, 0xFFFF};
> +#endif
>
>   /* We use screen_num below to refer to the real X screen number, but
>    * if we're using Xinerama, it will only be the Xinerama logical screen
> @@ -343,6 +346,25 @@ init_screen (rp_screen *s, int screen_num)
>   XSelectInput (dpy, s->help_window, KeyPressMask);
>
>   XSync (dpy, 0);
> +
> +#ifdef USE_XFT_FONT
> +  if (!XftColorAllocValue (dpy, DefaultVisual (dpy, screen_num),
> +                           DefaultColormap (dpy, screen_num), &rc, 
> &s->color))
> +    {
> +      PRINT_ERROR(("Failed to allocate font color\n"));
> +      s->ft_font = NULL;
> +    }
> +  else
> +    {
> +      s->ft_font = XftFontOpenName (dpy, screen_num, USE_XFT_FONT);
> +      if (!s->ft_font)
> +        {
> +          PRINT_ERROR(("Failed to open font\n"));
> +          XftColorFree (dpy, DefaultVisual (dpy, screen_num),
> +                        DefaultColormap (dpy, screen_num), &s->color);
> +        }
> +    }
> +#endif
>  }

Can you throw in an extra set of { } and include the rc var in that
one #ifdef? I think it's good to keep the number of ifdefs at a
minimum.

Otherwise, I think it looks good.

-Shawn