Re: PangoXft and PangoFT2 patch from hell

[Owen Taylor <otaylor redhat com>]

I've read through the patch now, except for the PangoFT2 parts. 
I assume that part is just doing the same thing as the PangoXft
part, and I'm less familiar with the code to begin with.

API comments
============

* The big question here is whether:

  void pango_font_map_load_fonts (PangoFontMap                 *fontmap,
				  const PangoFontDescription   *desc,
				  PangoFont                  ***fonts,
				  int                           n_font);

  is right API or whether we need something more abstract. The proposal
  Keith had at ALS that we should allow matches that don't match for the
  family as well if none of the families has the right glyphs would force
  us to go with something like:

   PangoFontSet *pango_font_map_load_fontset (PangoFontMap                 *fontmap,
				              const PangoFontDescription   *desc);
   PangoFont *pango_font_set_get_font        (PangoFontSet                 *font_set,
                                              guint                         wc);

  Advantages:

   - We could go with Keith's scheme without having to break the 
     backend API. The backend API is open for breakage within 1.x,
     but I'd rather not do it between now and 1.0.

   - Code cleanup ... we have a FontSet structure in pango-context, and
     PangoXftFontset, PangoFT2FontSet in the shaper engines that are
     used for caching.

  Disadvantages:

   - A reasonable amount work; it's basically just code reorg, but PangoFontSet 
     would have to be virtualized as yet another GObject, and for now
     we'd have a some code duplication between PangoFontSetDefault,
     PangoFontSetXft and PangoFontSetFT2.

   - Performance ... creating, refing, unrefing, GObjects is not all that fast
     (though it _needs_ to be).The Xft and FT2 backends could
     cache PangoFontSet objects, however -- they basically already cache
     equivalent structures. But see my note below about how they should
     be caching _unsized_ fontsets, not sized fontsets.

* int pango_font_map_load_fonts (PangoFontMap                 *fontmap,
				 const PangoFontDescription   *desc,
				 PangoFont                  ***fonts);

  gint pango_lookup_aliases (const char   *fontname,
		             char       ***families);

  Pango usage is uniformly:

  void pango_lookup_aliases (const char   *fontname,
		             char       ***families,
                             int          *n_families);


Implementation comments
=======================

* We shouldn't require LEX and YACC, for tarballs. So, we need to  
  check the results of AM_PROG_LEX, AC_PROG_YACC and define  
  some sort of @REBUILD_MINI_XFT@ based on this. (See handling 
  of @REBUILD@ in GTK+)

* Is it worth keeping XftFontmap->font_hash? Since in a normal
  program load_font() is never called, I think we should probably 
  omit this.

* I'm concerned that pango_xft_pattern_hash(), pango_xft_pattern_equal()
  are going to be a significant speed hits. I guess we'll cross
  that bridge when we profile it. It could be worked around 
  by using more complicated data structures to bypass this lookup
  most of the time, or we could look at ways of speeding this up
  (maybe special hooks in Xft? Not sure if hashing patterns is
  going to be any faster if done by Xft, however, since it doesn't
  know what are the interesting fields.)

* It's probably a bad thing that XftFontmap->fontset_hash both
  includes the size and is unbounded ... imagine changing 
  the point size continuously. 

  I think it would be better to save patterns without sizes
  (or with an arbitrary size) in ->fontset_hash, then 
  add in the size ourselves before trying to load each individual
  font.

  Plus, this means we wouldn't be doing the complicated family
  manipulations separately for each size.

  I'm not particularly worried about people who want to say
  "if family is times, set the size to 12, in their xftconfig", and
  PangoXft explicitely only includes fully scaleable fonts.

* In pango_xft_pattern_hash():

   if (XftPatternGetInteger (pattern, XFT_INDEX, 0, &i) != XftResultMatch)
     hash ^= i;

  isn't the result test backwards?

* Need doc comment for pango_font_map_load_fonts ().

* I don't think you meant to export:

  +void
  +pango_font_map_fontset_add_fonts (PangoFontMap          *fontmap,
  +				  GPtrArray             *fonts,
  +				  PangoFontDescription  *desc,
  +				  char                  *family)


Trivial comments
================

 *       patterns = g_new (PangoXftFontSet, 1);
  +      patterns->n_patterns = 0;
  +      max_patterns = 5;
  
  +	      if (patterns->n_patterns == max_patterns)
  +		{
  +		  max_patterns *= 2;
  +		  patterns->patterns = g_realloc (patterns->patterns, sizeof(XftPattern *) * max_patterns);
  +		}
  +

 I've gone over to using GArray/GPtrArray for this in almost all cases now.

 * Perhaps some/all of the _pango_xft functions should have standard
   doc comments; I've begun to do this for many _gtk functions in
   GTK+.

* The "match" argument to:

 PangoXftFont *
 _pango_xft_font_new (PangoFontMap               *fontmap,
-		     const PangoFontDescription *description,
-		     XftFont                    *xft_font)
+		     XftPattern                 *match)



 is perhaps a little odly named. Wouldn't "pattern" would make more sense?

* In pango-utils.c:

  +struct PangoAlias {
  +  gchar *alias;
  +  int n_families;
  +  gchar **families;
  +  gboolean visible; /* Do we want/need this? */
  +};
  +

  The opening '{' goes on the next line.

* gint pango_lookup_aliases (const char   *fontname,
		             char       ***families);

  'gint', 'gdouble' 'gchar' are _not_ used in pango, except for by accident.

* Bunch of spaces were added at the end of pango-utils.h