[Libreoffice] Horizontal glyph adjustments are ignored with ICU layout
devel at thanlwinsoft.org
Fri Jan 7 20:44:49 PST 2011
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
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.
More information about the LibreOffice