Re: OpenType Patches

[Owen Taylor <otaylor redhat com>]

Eric Mader <mader jtcsv com> writes:

> Index: pango-ot-ruleset.c
> ===================================================================
> RCS file: /cvs/gnome/pango/pango/opentype/pango-ot-ruleset.c,v
> retrieving revision 1.4
> diff -u -p -r1.4 pango-ot-ruleset.c
> --- pango-ot-ruleset.c	7 May 2002 20:38:50 -0000	1.4
> +++ pango-ot-ruleset.c	31 Aug 2002 01:51:17 -0000
> @@ -247,18 +247,25 @@ pango_ot_ruleset_shape (PangoOTRuleset  
>  	{
>  	  for (i = 0; i < result_string->length; i++)
>  	    {
> -	      int j;
> +	      FT_Pos x_pos = outgpos[i].x_pos, y_pos = outgpos[i].y_pos;

Style nit ... multiple assignments interleaved with declarations 
gets a bit messy, I'd prefer to see it broken up into two declarations.

> +	      PangoGlyphUnit x_adj = 0;
> +	      int back = i;
>  
> -	      glyphs->glyphs[i].geometry.x_offset += PANGO_UNITS_26_6 (outgpos[i].x_pos);
> -	      glyphs->glyphs[i].geometry.y_offset += PANGO_UNITS_26_6 (outgpos[i].y_pos);
> +	      while (outgpos[back].back != 0)
> +		{
> +		  back  -= outgpos[back].back;
> +		  x_pos += outgpos[back].x_pos;
> +		  y_pos += outgpos[back].y_pos;
> +		  x_adj += glyphs->glyphs[back].geometry.width;
> +		}

Don't we have to take into account glyphs->glyphs[].geometry.width
for _all_ the characters intermediate between the current character 
and the base character? 

> +	      glyphs->glyphs[i].geometry.x_offset += PANGO_UNITS_26_6(x_pos) - x_adj;
> +	      glyphs->glyphs[i].geometry.y_offset += PANGO_UNITS_26_6(y_pos);
>  
> -	      for (j = i - outgpos[i].back; j < i; j++)
> -		glyphs->glyphs[i].geometry.x_offset -= glyphs->glyphs[j].geometry.width;
> -	    
>  	      if (outgpos[i].new_advance)
>  		/* Can't set new x offset for marks, so just make sure not to increase it.
>  		   Can do better than this by playing with ->x_offset. */
> -		glyphs->glyphs[i].geometry.width = 0;
> +		glyphs->glyphs[i].geometry.width  = PANGO_UNITS_26_6(outgpos[i].x_advance);

Do you understand the FIXME here? I don't, really. The comment applies
to the '= 0', so it certainly isn't literally relevant with your changes.

>  	      else
>  		glyphs->glyphs[i].geometry.width += PANGO_UNITS_26_6(outgpos[i].x_advance);
>  	    }

In general these changes look like they make sense, though. (The 
LangSys part of the patch looks fine to commit.)

Regards,
                                        Owen