From 1a4b3201e747c45d12044105d86cfc4a3f11af3c Mon Sep 17 00:00:00 2001 From: Jean-Marc Lasgouttes Date: Mon, 3 Mar 2014 15:29:37 +0100 Subject: [PATCH] Update README and do a small code cleanup --- 00README_STR_METRICS_BRANCH | 83 ++++++++++++++++++++++++++----------- src/TextMetrics.cpp | 13 +++--- 2 files changed, 64 insertions(+), 32 deletions(-) diff --git a/00README_STR_METRICS_BRANCH b/00README_STR_METRICS_BRANCH index 1983dfe984..1bb70261c6 100644 --- a/00README_STR_METRICS_BRANCH +++ b/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 -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. diff --git a/src/TextMetrics.cpp b/src/TextMetrics.cpp index 9e775e0def..dd5b12c568 100644 --- a/src/TextMetrics.cpp +++ b/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 @@ -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 -- 2.39.2