[Libreoffice-commits] core.git: 2 commits - vcl/generic vcl/source

[Khaled Hosny]

 vcl/generic/glyphs/gcach_layout.cxx |   31 ++++++++++++++++++++++---------
 vcl/source/gdi/sallayout.cxx        |   21 +++++++++++++++++++++
 2 files changed, 43 insertions(+), 9 deletions(-)

New commits:
commit 71077148d442b3bbfeefd9a572942946c6a95823
Author: Khaled Hosny <khaledhosny at eglug.org>
Date:   Wed Oct 30 09:34:38 2013 +0200

    fdo#70968: Incorrect rendering of Devanagari short 'i' vowel
    
    It seems that some Indic fonts assign 'mark' glyph class to combining
    spacing marks (spacing not non spacing) so my reliance on the glyph
    class to set the IS_DIACRITIC flags broke those fonts. This is a bandaid
    to get around the issue, plus some long rant! (at this rate, I'll be
    writing "The VCL haters handbook" pretty soon).
    
    Change-Id: I3ff892acf746d50182573f94e7e8c3c6f9464ae0

diff --git a/vcl/generic/glyphs/gcach_layout.cxx b/vcl/generic/glyphs/gcach_layout.cxx
index 4673931..7a8bfc9 100644
--- a/vcl/generic/glyphs/gcach_layout.cxx
+++ b/vcl/generic/glyphs/gcach_layout.cxx
@@ -456,20 +456,36 @@ bool HbLayoutEngine::layout(ServerFontLayout& rLayout, ImplLayoutArgs& rArgs)
             if (bInCluster)
                 nGlyphFlags |= GlyphItem::IS_IN_CLUSTER;
 
+            // The whole IS_DIACRITIC concept is a stupid hack that was
+            // introduced ages ago to work around the utter brokenness of the
+            // way justification adjustments are applied (the DXArray fiasco).
+            // Since it is such a stupid hack, there is no sane way to directly
+            // map to concepts of the "outside" world, so we do some rather
+            // ugly hacks:
+            // * If the font has a GDEF table, we check for glyphs with mark
+            //   glyph class which is sensible, except that some fonts
+            //   (fdo#70968) assign mark class to spacing marks (which is wrong
+            //   but usually harmless), so we try to sniff what HarfBuzz thinks
+            //   about this glyph by checking if it gives it a zero advance
+            //   width.
+            // * If the font has no GDEF table, we just check if the glyph has
+            //   zero advance width, but this is stupid and can be wrong. A
+            //   better way would to check the character's Unicode combining
+            //   class, but unfortunately glyph gives combining marks the
+            //   cluster value of its base character, so nCharPos will be
+            //   pointing to the wrong character (but HarfBuzz might change
+            //   this in the future).
             bool bDiacritic = false;
             if (hb_ot_layout_has_glyph_classes(mpHbFace))
             {
                 // the font has GDEF table
-                if (hb_ot_layout_get_glyph_class(mpHbFace, nGlyphIndex) == HB_OT_LAYOUT_GLYPH_CLASS_MARK)
+                bool bMark = hb_ot_layout_get_glyph_class(mpHbFace, nGlyphIndex) == HB_OT_LAYOUT_GLYPH_CLASS_MARK;
+                if (bMark && pHbPositions[i].x_advance == 0)
                     bDiacritic = true;
             }
             else
             {
                 // the font lacks GDEF table
-                // HACK: if the resolved glyph advance is zero assume it is a
-                // combining mark.  The whole IS_DIACRITIC concept is a hack to
-                // fix the other hacks we use to second-guess glyph advances in
-                // ApplyDXArray and the likes and it needs to die
                 if (pHbPositions[i].x_advance == 0)
                     bDiacritic = true;
             }
diff --git a/vcl/source/gdi/sallayout.cxx b/vcl/source/gdi/sallayout.cxx
index f395936..450ec232 100644
--- a/vcl/source/gdi/sallayout.cxx
+++ b/vcl/source/gdi/sallayout.cxx
@@ -1018,6 +1018,27 @@ void GenericSalLayout::AdjustLayout( ImplLayoutArgs& rArgs )
 
 // -----------------------------------------------------------------------
 
+// This DXArray thing is one of the stupidest ideas I have ever seen (I've been
+// told that it probably a one-to-one mapping of some Windows 3.1 API, which is
+// telling). To justify a text string, Writer calls OutputDevice::GetTextArray()
+// to get an array that maps input characters (not glyphs) to their absolute
+// position, GetTextArray() in turn calls SalLayout::FillDXArray() to get an
+// array of character widths that it converts to absolute positions.
+//
+// Writer would then apply justification adjustments to that array of absolute
+// character positions and return to OutputDevice, which eventually calls
+// ApplyDXArray(), which needs to extract the individual adjustments for each
+// character to apply it to corresponding glyphs, and since that information is
+// already lost it tries to do some heuristics to guess it again. Those
+// heuristics often fail, and have always been a source of all sorts of weird
+// text layout bugs, and instead of fixing the broken design a hack after hack
+// have been applied on top of it, making it a complete mess that nobody
+// understands.
+//
+// As you can see by now, this is utterly stupid, why Writer does not just send
+// us directly the advance width transformations it wants to apply to each
+// character instead of this whole mess?
+
 void GenericSalLayout::ApplyDXArray( ImplLayoutArgs& rArgs )
 {
     if( m_GlyphItems.empty())
commit 8886336141dcbf21961d0c86a82f6224d7ac7a31
Author: Khaled Hosny <khaledhosny at eglug.org>
Date:   Wed Oct 30 07:10:43 2013 +0200

    Simplify
    
    Change-Id: I30ab912b96fffa181b5cf062ecdf80faec615ec3

diff --git a/vcl/generic/glyphs/gcach_layout.cxx b/vcl/generic/glyphs/gcach_layout.cxx
index e207c5e..4673931 100644
--- a/vcl/generic/glyphs/gcach_layout.cxx
+++ b/vcl/generic/glyphs/gcach_layout.cxx
@@ -56,10 +56,7 @@ void ServerFontLayout::DrawText( SalGraphics& rSalGraphics ) const
 
 bool ServerFontLayout::LayoutText( ImplLayoutArgs& rArgs )
 {
-    ServerFontLayoutEngine* pLE = mrServerFont.GetLayoutEngine();
-    assert(pLE);
-    bool bRet = pLE ? pLE->layout(*this, rArgs) : false;
-    return bRet;
+    return mrServerFont.GetLayoutEngine()->layout(*this, rArgs);
 }
 
 // -----------------------------------------------------------------------