From: Paul Rohr (paul@abisource.com)
Date: Thu May 09 2002 - 19:06:37 EDT
At 09:17 PM 5/9/02 +0100, Tomas Frydrych wrote:
>We could, of course, do this, though I personally do not see much
>need for it. If you look into the sources,
Tomas,
I've taken your advice and grepped through all the BIDI_ changes to assess
their potential impact (if any) on LTR people who don't use any of the bidi
features.
fl_BlockLayout::_doInsertTextSpan()
-----------------------------------
Adds per-character FriBidi downcall to see if the span needs to be
subdivided into runs of different directions. Unavoidable, no?
fp_Line::layout()
-----------------
In fact, the iX erasure stuff looks like it might *fix* problems in the
non-BIDI build. (I haven't checked, though.)
The only other notable change is the stuff in the tab-handling logic,
which needs to switch from the pRun->getNext() idiom to the visual
equivalent. I briefly wondered whether it might be worth creating a new
iterator to encapsulate it, but the only ones I could think of would be
slower.
fp_Line::calculateWidthOfTrailingSpaces()
fp_Line::calculateWidthOfTrailingSpacesInLayoutUnits()
------------------------------------------------------
Again, just an iterator swap.
fp_Line::_splitRunsAtSpaces()
-----------------------------
Calls new _createMapOfRuns() function if any runs got split. Fortunately
that function is optimized for the non-BiDi case so that the only impact
would be to occasionally grow the maps.
SUGGESTION: Can you move the LTR-only logic earlier in the function so
even this growth step gets skipped?
fp_Line::_getRunLogIndx()
fp_Line::_getRunVisIndx()
-------------------------
Already optimized for the "no RTL" case. Nice.
fp_Run::setNext()
fp_Run::setPrev()
-----------------
If I understand the comments here, downstream logic will help avoid
triggering the expensive bRefresh stuff in non-BIDI cases. I didn't check
to confirm this though.
fp_TextRun::fp_TextRun()
fp_TextRun::~fp_TextRun()
-------------------------
I like the idea of a static cache for the prefs values needed. However, I
was a bit surprised to see that you're using a timer to poll for changes.
Is this an idiom we're using elsewhere, or would it be better to register
a prefs listener to update that cache? (I'm just skimming code, and
haven't thought through the timing issues here.)
fp_TextRun::mergeWithNext()
---------------------------
At the bottom, there's a SMART_RUN_MERGING block which isn't also guarded
by a BIDI_ENABLED block. Fortunately, nobody's tried screwing up the
#defines to trigger this yet. ;-)
fp_TextRun::simpleRecalcWidth()
-------------------------------
By taking advantage of the draw buffer, the BIDI version can avoid walking
and summing frag lengths. Having debugged that idiom when we first
introduced it, I'm not at all sad to see it go. :-)
SUGGESTION: Only do the SetFont if s_bUseContextGlyphs?
fp_TextRun::recalcWidth()
-------------------------
A quick peek suggests that the LTR-only case gets faster, if anything.
fp_TextRun::_draw()
-------------------
I've been hoping that the _refreshDrawBuffer() would have been triggered
before this point. Is this just here as a failsafe for race conditions,
or do you *intend* to do the refresh at draw time? Can we avoid ever
creating the draw buffer for portions of the document which are never
displayed?
Further down, does it hurt that the getNextVisual() and getPrevVisual()
implementations don't confirm that the run found is on the same line?
It looks like the BIDI variant would redraw adjacent italic chars on a
different line.
SUGGESTION: Fix by adding the line checks to the guards on each of the
draw blocks.
fp_TextRun::_drawPart()
-----------------------
This implementation changes a lot, insofar as the work gets offloaded to
_refreshDrawBuff(), which is now the caller's responsibility. Without
understanding the performance implications, I see why it'd be necessary
for bidi drawing.
Possible bugs ... drawbuff not refreshed by all callers, but that's
probably been fixed already. And if not, then the catchall in _draw()
will certainly hide any such problems. ;-)
pt_PieceTable::_loadBuiltinStyles()
-----------------------------------
SUGGESTION: It'd be easier to read the BIDI variants of the "Normal"
style if the properties are listed in the same order.
And that's all folks!
bottom line
===========
Definitely go ahead and strip out the ifdef guards. After one pass through
all that code, the only possible issues I found above all seem quite minor.
Indeed, if I'd realized from the get-go how radically similar the LTR-only
codepath is, I probably would never have objected in the first place. For
all the messy ifdef-ness, this should be a very very clean switchover.
Great work, Tomas!
Paul,
who loves reading mature code
This archive was generated by hypermail 2.1.4 : Thu May 09 2002 - 19:08:27 EDT