Re: [Libreoffice] Horizontal glyph adjustments are ignored with ICU layout

Hi Khaled, Michael,

On 07/01/2011 8:21 PM, Michael Meeks wrote:
>         Drat; we took ages to reply. I suspect Thorsten might be able to help,
> but failing that the best text expert I'm aware of is Tim (CC'd) who may
> be able to give some advice. SIL's Graphite integration would
> undoubtedly have fallen over the same sort of problems, and no doubt he
> can give some pointers to how to work around it - Tim ? :-)

I've been working on the Graphite integration more recently, so I'll try 
to make a few comments.
> > Anyway, it turned out that the issue is not specific to kerning nor
> > Arabic, but affects all horizontal glyph positioning in the ICU layout
> > path; the problem does not show on Windows nor with Graphite fonts and
> > of course not with con-CTL.
It certainly sounds like it is worth fixing.
> > The X adjustment of glyph widths is simply ignored, and glyphs are drawn
> > stacked after each other baased on their original width, which results in
> > kerning being ignored as well as other forms of glyph positioning like
> > cursive anchors, however vertical glyph positions (in the Y access) are
> > OK.

It depends on the rendering path, different parts of libo call the 
layout methods in different ways.
> > In source/glyphs/gcach_layout.cxx, ICU's layoutChars() is called and new
> > glyph indices and positions are returned, which is then fed into
> > SalLayout in the form of GlyphItem's. Though GlyphItem has maLinearPos
> > which holds its absolute position, many places in the code re-calculate
> > glyph position from its mnOrigWidth (original glyph width) and mnNewWidth
> > (width after adjustments), but the ICU path simply sets mnNewWidth to
> > mnOrigWidth since ICU layout engine does not return individual glyph
> > widths, resulting in failure of glyph position re-calculation which
> > manifests in this bug.
> >
> > As a prove of concept, the attached patch trays to set mnNewWidth in a
> > very hackish way by subtracting current glyph position from the of next
> > non-diacritic (+ve) glyph. It does indeed fix the problem (see the
> > attached screenshots), but it looks very unreliable to me.

I think subtracting the glyph positions is a reasonable approach, 
something similar is done in the graphite integration code to get the 
numbers into a form that works with libo's logic.
> > May be a
> > cleaner solution is to re-implement all the broken virtual methods to
> > eliminate the re-calculation part.

Do you mean implement ICU/ServerFontLayout specific versions of the 
methods that ServerFontLayout is currently using directly from 
GenericSalLayout? I think that would be quite a lot of work and you may 
still need something like mnNewWidth calculated in a similar way to your 
patch. I admit that GraphiteLayout does reimplement those methods so if 
it turns out there is an assumption in GenericSalLayout which doesn't 
fit well with ICU then that might still make sense.

The recalculations are done in several situations such as for 
justification, text expansion, text contraction. In writer the text is 
rendered once with a small font size and once with a large font size. It 
then does some calculations to try to give a more accurate WYSIWYG 
positioning of the text on screen which results in small adjustments 
being applied to the glyph positions (but the request to adjust is based 
on characters). It is probably the later which is interacting with the 
incorrect mnNewWidth to give the bad positions.

Looking more carefully at the patch, there are a few points that may 
need more consideration:

a) It may be possible for glyphs to be out of order, in which case

nNewWidth = pNextPos->fX - pPos->fX;

might result in a negative value even when the nominal nNextGlyphWidth 
is positive, which could cause problems.

b) The glyphs are resorted a bit later in IcuLayoutEngine::operator() 
with a rLayout.SortGlyphItems(); call. This might upset the new width 
values. However, it seems to only reorder diacritics and those are 
skipped for the width calculation, so that is probably alright.

c) For the last glyph in a run, pNextPos will still be valid (see 
LayoutEngine::getGlyphPositions docs), so I think it should be used for 
the nNewWidth calculation.

It should be tested with some CTL scripts including reordering and lots 
of diacritics to check the edge cases are covered. The width will also 
affect font fallback (calls from MultiSalLayout), so that needs to be 
tested as well.

Best regards,

Keith