]> git.lyx.org Git - features.git/commitdiff
Update README and do a small code cleanup
authorJean-Marc Lasgouttes <lasgouttes@lyx.org>
Mon, 3 Mar 2014 14:29:37 +0000 (15:29 +0100)
committerJean-Marc Lasgouttes <lasgouttes@lyx.org>
Tue, 8 Jul 2014 19:23:58 +0000 (21:23 +0200)
00README_STR_METRICS_BRANCH
src/TextMetrics.cpp

index 1983dfe98424b229304baf90b617c754cb99518d..1bb70261c6547e190f1721e10cf0fe3305201590 100644 (file)
@@ -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
-ligatures) and better performance than what we have with
-force_paint_single_char.
+
+!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
+PLEASE DO NOT DO WORK ON TOP OF THIS BRANCH.
+I INTEND TO REWRITE HISTORY LATER!!!
+!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
+
+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
-  metrics are computed. The list of elements is stored in the row
-  object in visual ordering, not logical.
+* Change breakRow operation to operate at strings level to compute
+  metrics The list of elements is stored in the row object in visual
+  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.
 
+* Profile and see how performance can be improved.
+
+
+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. 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.
+  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.
 
-* Profile and see how performance can be improved.
+  This would allow to get rid of the Bidi code.
 
 
-Difference in behavior (aka bug fixes)
+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.
index 9e775e0defe35746c50bc8cb11619018d3a6b9a1..dd5b12c568d41fe4039185d38b257b4bc999812d 100644 (file)
@@ -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);
-
-       double x = row.x;
+       pos_type const pos = sl.pos();
 
        /**
         * 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