[Libreoffice] Horizontal glyph adjustments are ignored with ICU layout

Hi every one,

This my first message to the list, hopping it will be a fruitful
experience :)

I was trying to debug fdo#31016[1] since it breaks almost all my fonts,
it toke me few months but I think I finally got the root of it (I
started working on it few weeks before libreoffice was announced, what a
nice code base ;).

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.

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.

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. May be a
cleaner solution is to re-implement all the broken virtual methods to
eliminate the re-calculation part.

Hope this helps, and sorry for the broken terminology as I'm not really
a developer and less of C++ one (I can only do much of C++ that looks
like C, which I don't really "speak" either).

[1] https://bugs.freedesktop.org/show_bug.cgi?id=31016

Regards,
 Khaled

-- 
 Khaled Hosny
 Arabic localiser and member of Arabeyes.org team
 Free font developer

diff --git a/vcl/source/glyphs/gcach_layout.cxx b/vcl/source/glyphs/gcach_layout.cxx
index cda8749..7531d2b 100644
--- a/vcl/source/glyphs/gcach_layout.cxx
+++ b/vcl/source/glyphs/gcach_layout.cxx
@@ -534,12 +534,35 @@ bool IcuLayoutEngine::operator()( ServerFontLayout& rLayout, ImplLayoutArgs& 
rAr
             aNewPos = Point( (int)(pPos->fX+0.5), (int)(pPos->fY+0.5) );
             const GlyphMetric& rGM = rFont.GetGlyphMetric( nGlyphIndex );
             int nGlyphWidth = rGM.GetCharWidth();
+            int nNewWidth = nGlyphWidth;
             if( nGlyphWidth <= 0 )
                 bDiacritic |= true;
             // #i99367# force all diacritics to zero width
             // TODO: we need mnOrigWidth/mnLogicWidth/mnNewWidth
-            else if( bDiacritic )
-                nGlyphWidth = 0;
+            else {
+                if( bDiacritic ) {
+                    nGlyphWidth = 0;
+                    nNewWidth = 0;
+                } else {
+                    // Hack, find next +ve width glyph and calculate current
+                    // glyph width by substracting the two posituons
+                    const IcuPosition* pNextPos = pPos+1;
+                    for ( int j = i + 1; j < nRawRunGlyphCount; ++j, ++pNextPos) {
+                        LEGlyphID nNextGlyphIndex = pIcuGlyphs[j];
+
+                        if( (nNextGlyphIndex == ICU_MARKED_GLYPH) || (nNextGlyphIndex == 
ICU_DELETED_GLYPH) )
+                            continue;
+
+                        const GlyphMetric& rNextGM = rFont.GetGlyphMetric( nNextGlyphIndex );
+                        int nNextGlyphWidth = rNextGM.GetCharWidth();
+
+                        if (nNextGlyphWidth>0) {
+                            nNewWidth = pNextPos->fX - pPos->fX;
+                            break;
+                        }
+                    }
+                }
+            }
 
             // heuristic to detect glyph clusters
             bool bInCluster = true;
@@ -593,7 +616,9 @@ bool IcuLayoutEngine::operator()( ServerFontLayout& rLayout, ImplLayoutArgs& rAr
                 nGlyphFlags |= GlyphItem::IS_DIACRITIC;
 
             // add resulting glyph item to layout
-            const GlyphItem aGI( nCharPos, nGlyphIndex, aNewPos, nGlyphFlags, nGlyphWidth );
+            GlyphItem aGI( nCharPos, nGlyphIndex, aNewPos, nGlyphFlags, nGlyphWidth );
+            aGI.mnNewWidth = nNewWidth;
+            //printf("%ld: %d=>%d\n", aGI.mnGlyphIndex, aGI.mnOrigWidth, aGI.mnNewWidth);
             rLayout.AppendGlyph( aGI );
             ++nFilteredRunGlyphCount;
             nLastCharPos = nCharPos;

Attachment: before.png
Description: PNG image

Attachment: after.png
Description: PNG image