Update README and do a small code cleanup

00README_STR_METRICS_BRANCH

@@ -1,21 +1,34 @@
-This branch is where I (jmarc) try to implement string_wise metrics
+
-computation. This is done through a series of cleanups. The goal is to
+!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
-have both good metrics computation (and font with proper kerning and
+PLEASE DO NOT DO WORK ON TOP OF THIS BRANCH.
-ligatures) and better performance than what we have with
+I INTEND TO REWRITE HISTORY LATER!!!
-force_paint_single_char.
+!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
+
+This branch is where I (jmarc) try to implement string-wise metrics
+computation. The goal is to have both good metrics computation (and
+font with proper kerning and ligatures) and better performance than
+what we have with force_paint_single_char. Moreover there has been
+some code factorization in TextMetrics, where the same row-breaking
+algorithm was basically implemented 3 times.
 
 Currently everything is supposed to work for LTR text, and RTL text
 should work too except possibly metrics with Arabic and Hebrew fonts.
-We'll see what to do after some feedback.
+
+When KEEP_OLD_METRICS_CODE is defined in TextMetrics.cpp, the new code
+is tested against the old one in getPosNearX and cursorX. This can be
+helpful when looking for discrepancies between the algorithms. Note
+that this only makes sense when force_paint_single_char=true, since
+this enforces char-by-char metrics computation.
 
 What is done:
 
 * Make TextMetrics methods operate on Row objects: breakRow and
   setRowHeight instead of rowBreakPoint and rowHeight.
 
-* Change breakRow operation to operate on text strings on which
+* Change breakRow operation to operate at strings level to compute
-  metrics are computed. The list of elements is stored in the row
+  metrics The list of elements is stored in the row object in visual
-  object in visual ordering, not logical.
+  ordering, not logical. This will eventually allow to get rid of the
+  Bidi class.
 
 * rename getColumnNearX to getPosNearX (and change code accordingly).
   It does not make sense to return a position relative to the start of
@@ -30,38 +43,58 @@ What is done:
 
 Next steps:
 
-* check what happens with arabic and/or hebrew text. There may be some
+* check what happens with Arabic and/or Hebrew text. There may be some
   problems related to compose characters. I suspect that some code is
   needed in FontMetrics::width.
 
+* investigate whether RtL strings could be drawn on a string-wise basis.
+
+* investigate whether Row::SEPARATOR elements could be used only in
+  justified text. This would speed-up painting in other cases by
+  lowering the number of strings to draw.
+
+* get lots of testing.
+
 * Get rid of old code in cursorX and getColumnNearX; it has been
   kept for comparison purpose, guarded with KEEP_OLD_METRICS_CODE in
   order to check computations.
 
-* Re-implement row painting using row elements. This is not difficult
-  in principle, but the code is intricate and needs some careful
-  analysis. First thing that needs to be done is to break row elements
-  with the same criterions. Currently TextMetrics::breakRow does not
-  consider on-the-fly spellchecking and selection changes, but it is
-  not clear to me that it is required.
-
 * Profile and see how performance can be improved.
 
 
-Difference in behavior (aka bug fixes)
+Steps for later (aka out of the scope of this branch):
+
+* Re-implement row painting using row elements. This is not difficult
+  in principle, but the code is intricate and needs some careful
+  analysis. The first thing that needs to be done is to break row
+  elements with the same criteria. Currently TextMetrics::breakRow
+  does not consider on-the-fly spell-checking and selection changes,
+  but it is not clear to me that it is required. Moreover, this thing
+  would only work if we are sure that the Row object is up-to-date
+  when drawing happens. This depends on the update machinery.
+
+  This would allow to get rid of the Bidi code.
+
+
+Known bugs:
+
+* in RtL paragraphs, the end-of-paragraph marker moves the row to the
+  right (ticket #9040, already present in master).
+
+* there are still difference in what breaks words. In particular,
+  RowPainter breaks strings at: selection end, spell-checking
+  extremities. This seems to be harmless.
+
+* when clicking in the right margin, GetColumnNearX does not return
+  the same value as before. I am not sure whether this is important.
+
+
+Other differences in behavior (aka bug fixes):
 
 * end of paragraph markers metrics are computed with the font of the
   actual text, not default font.
 
-* When cursor is after a LTR separator just before a RTL chunk, the
+* When cursor is after a LtR separator just before a RtL chunk, the
   cursor position is computed better with the new code.
 
 
-Other differences (aka real bugs)
-
-* there are still difference in what breaks words. In particular,
-  RowPainter breaks strings at: selection end, spellchecking
-  extremities.
-
-* when clicking in the right margin, GetColumnNearX does not return
-  the same value as before.

src/TextMetrics.cpp

@@ -15,7 +15,7 @@
  * Full author contact details are available in file CREDITS.
  */
 
-#define KEEP_OLD_METRICS_CODE 1
+//#define KEEP_OLD_METRICS_CODE 1
 
 #include <config.h>
 
@@ -1601,15 +1601,12 @@ int TextMetrics::cursorX(CursorSlice const & sl,
 		bool boundary) const
 {
 	LASSERT(sl.text() == text_, return 0);
-	pit_type const pit = sl.pit();
-	pos_type pos = sl.pos();
 
-	ParagraphMetrics const & pm = par_metrics_[pit];
+	ParagraphMetrics const & pm = par_metrics_[sl.pit()];
 	if (pm.rows().empty())
 		return 0;
 	Row const & row = pm.getRow(sl.pos(), boundary);
-
+	pos_type const pos = sl.pos();
-	double x = row.x;
 
 	/**
 	 * When boundary is true, position i is in the row element (pos, endpos)
@@ -1625,9 +1622,10 @@ int TextMetrics::cursorX(CursorSlice const & sl,
 	if (row.empty()
 	    || (row.begin()->font.isVisibleRightToLeft()
 		&& pos == row.begin()->endpos))
-		return int(x);
+		return int(row.x);
 
 	Row::const_iterator cit = row.begin();
+	double x = row.x;
 	for ( ; cit != row.end() ; ++cit) {
 		if (pos + boundary_corr >= cit->pos
 		    && pos + boundary_corr < cit->endpos) {
@@ -1643,6 +1641,7 @@ int TextMetrics::cursorX(CursorSlice const & sl,
 			<< ", " << boundary_corr << "): NOT FOUND! " << row);
 
 #ifdef KEEP_OLD_METRICS_CODE
+	pit_type const pit = sl.pit();
 	Paragraph const & par = text_->paragraphs()[pit];
 
 	// Correct position in front of big insets