Re: [patch] Rework of Pango integration

[Jody Goldberg <jody gnome org>]

On Thu, Jun 03, 2004 at 08:54:58PM -0400, Owen Taylor wrote:
> 
>  * I've changed how PangoFont => GnomeFont works so that instead of
>    doing it at a "naming" level, it actually maps according to font
>    file.

In the longer term do you see any value in maintaining GnomeFont vs
PangoFont ? or would you rather just see us move to a cairo backend ?
 
>  * I'd consider this patch "done" other than review comments; 
>    it's reasonably well documented and commented and should
>    be pretty much complete. The one thing I can think of that
>    needs fixing is:
> 
>     http://bugzilla.gnome.org/show_bug.cgi?id=114431
> 
>    But that mostly comes up for menu-style partial-word underlines.

I don't see that as terribly significant for the time being.
 
>  * The patch makes gnome-print require CVS head Pango but for a
>    pretty surface reason; all that is being used is the
>    new underline and strikethrough font metrics. It would be
>    pretty easy to add a configure check and fallbacks for that.

It will depend on the pango release schedule.  If you still plan to
release a new stable version before gnome-2.8 then it seems
unnecessary.  This code isn't going to be backported to gp-2.6.

>  * Code is written for simplicity rather than performance; some
>    of the gnome_print_gsave/grestore could be optimized away.

Also not worth the bother.  There are many areas that could easily
be optimized for much larger gains.  We care about the size of the
generated output, but the save/restores don't bloat that
significantly

> Index: doc/reference/tmpl/gnome-print-pango.sgml
> ===================================================================
Docs too !  You do nice work.

> +/**
> + * gnome_print_pango_update_context:
> + * @context: a #PangoContext from gnome_print_pango_create_context().
> + * @gpc: a #GnomePrintContext
> + * 
> + * Update a context so that layout done with it reflects the
> + * current state of @gpc. In general, every time you use a
> + * #PangoContext with a different #GnomePrintContext, or
> + * you change the transformation matrix of the #GnomePrintContext
> + * (other than pure translations) you should call this function.
> + * You also need to call pango_layout_context_changed() for any
> + * #PangoLayout objects that exit for the #PangoContext.

'Change the transformation matrix' ?
I'll assume that is not necessary for translation.

> +static void
> +get_item_properties (PangoItem *item, ItemProperties *properties)
> +{
...
> +		switch (attr->klass->type) {
> +		case PANGO_ATTR_UNDERLINE:
> +		case PANGO_ATTR_STRIKETHROUGH:
> +		case PANGO_ATTR_FOREGROUND:
> +		case PANGO_ATTR_BACKGROUND:
> +		case PANGO_ATTR_SHAPE:
> +		case PANGO_ATTR_RISE:
> +		default:
Where is ATTR_SCALE handled ?

> +void 
> +gnome_print_pango_layout (GnomePrintContext *gpc, PangoLayout *layout)
> +{
> +	PangoLayoutIter *iter;
> +	
> +	g_return_if_fail (GNOME_IS_PRINT_CONTEXT (gpc));
> +	g_return_if_fail (PANGO_IS_LAYOUT (layout));

A sanity check to ensure that the layout is associated with a 
context that has a PangoFT2FontMap seems like a good idea.  Can we
get more specific to ensure that it's really one of the gnome-print
created fontmaps with hinting disabled ?

Other than those minor questions this looks good to go.