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

2 files changed, 42 insertions, 5 deletions

diff --git a/vcl/generic/glyphs/gcach_layout.cxx b/vcl/generic/glyphs/gcach_layout.cxx
index 4673931ffaba..7a8bfc982952 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 f395936b34d7..450ec2325731 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())