Re: OpenType Patches

[Eric Mader <mader jtcsv com>]

At 09:07 AM 9/6/2002, Owen Taylor wrote:
> > The code in ftstrtto.c doesn't mess with the widths at all; it
> > positions the accents at the position of the base character plus
> > [x_pos, y_pos]. (I tried to do exactly that in this patch, but the
> > positions aren't set when this code runs...)
>
> Are you just referring to the Xft bug, or is this something else?

No. all of the glyphs->glyphs[i].geometry.x_offset values seemed to be zero 
on entry to this function. I've just been looking through the code, and it 
seems to explicitly set them this way. (in indic_engine_shape in 
indic-xft.c - I think I copied this from arabic-xft.c but I don't remember 
for sure - the same code is there too...) I thought that the x_offset and 
y_offset values would have the default (x, y) position of each glyph. I 
don't see any code that does anything other than apply the adjustments, so 
I guess there's something I'm not seeing. Can someone enlighten me?

> > So, in theory, I think you're right about needing to look at all of
> > the intermediate widths. On the other hand, all of the intermediate
> > glyphs will be marks of one kind or another, so they should all have
> > zero widths.
>
> Since taking all the intermediate glyphs into account shouldn't be
> hard, I'd rather we did that then count on all the intermediate
> widths being zero.

Sure, unless I can figure out where the x, y positions are; using the 
position of the base glyph directly seems like the best way to go. Using 
the widths of the intermediate glyphs seems like a bit of a hack, somehow...

> > > > +           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);
> > > >           }
> >
> > No, I don't understand the comment here. I was tempted to remove it,
> > but figured I should leave it in case someone else knows what it
> > means. My understanding of the new_advance flag is that it means that
> > x_advance is a new advance width rather than an adjustment to the
> > existing advance width. (This is done because it makes some of the
> > GPOS calculations a bit simpler)
>
> Lets just kill the comment then; an inaccurate comment is worse than
> no comment.

OK. I was afraid I was missing something important. I suppose if the code 
seems to work, then I can be pretty sure that I wasn't :-)

Any rush on committing these changes? I'm a bit pressed for time at the 
moment and may not be able to get them for a bit.

> Regards,
>                                         Owen

Regards,
Eric