Subject: BiDi patch comments
From: Jesper Skov (jskov@redhat.com)
Date: Sat Dec 30 2000 - 04:39:27 CST
Just a few comments...
Bugger! I've gotten to fp_Line::_createMapOfRuns(), but haven't read
the code to understand what it's doing. And I really have to run now.
But the below should be a start :)
Jesper
/****/
You want to separate the BiDi patch from your string assembler
optimization patch.
/****/
+#ifdef BIDI_ENABLED
+ UT_sint32 m_iStartPosition;
+ UT_sint32 m_startPositionLayoutUnits;
+#endif
should be
+#ifdef BIDI_ENABLED
+ UT_sint32 m_iStartPosition;
+ UT_sint32 m_iStartPositionLayoutUnits;
+#endif
(as should be the variables in other classes, but that's not your
problem)
/****/
I wish you'd use an enum for the dominant direction type. Bool is fine
if you know the code intimately, but an enum makes for self
documenting code.
Same goes for the return type of getVisDirection()
/****/
+ * only one instance for all the instances of the class; public becuase it must be inicialsed
Do you mean initialized? I think you do. You have the same (or
similar) misspelling a few places.
/****/
I'm not sure if this is the case, but won't
fp_Line::_createMapOfRuns() end up having a monstrously big
s_pMapOfRuns after a load/import?
The reason I ask is that on load/import all Runs of a paragraph is
stuffed into the same line, whereafter it is split up in mutiple lines
as necessary for the flow of the text.
If that does indeed affect your code, you'll end up with a large array
(or large arrays in the non-static version) which is never fully used
after the initial load.
/****/
+ //we will be earsing from invalid coordinances.
Typo : erasing.
/****/
+ //if we follow a run that is consistent with the direciton of the para, then so will be this
+ //(dont have to worry about the i-1 since this will never happen on position 0
+ if(s_pMapOfRuns[i-1] == 0)
+ s_pMapOfRuns[i] = 0;
Why? Can't a space be the first thing on a line?
/****/
+ if(j == count) s_pMapOfRuns[i] = s_pMapOfRuns[i-1]; //last run on the line, will have the dir of the preceedign run
dir -> direction
preceedign -> preceding
/****/
+ while(s_pMapOfRuns[i] && (i < count))
+ ++i;
+ --i;
Bad indentation on the last line.
/****/
+ // TODO ? this would be much more efficient in inline Assembler
Maybe. But don't even consider it at this time.
/****/
This archive was generated by hypermail 2b25 : Sat Dec 30 2000 - 04:39:30 CST