From b8170e0e01f486fefb94b19566e24dd391419c12 Mon Sep 17 00:00:00 2001 From: Jean-Marc Lasgouttes Date: Fri, 21 Mar 2014 11:56:42 +0100 Subject: [PATCH] Fix various selection-related problems All these problems are related to what happens at the extreme points of rows * since VIRTUAL elements have a width but no contents, they have to be treated specially at some places. It would have been better to avoid testing for them explicitly, but I did not find a way. * Improve and cleanup the code in breakRow and fix in passing a crash when clicking on the right of an incomplete MARGIN_MANUAL paragraph. * improve the computation of row width in TextMetrics::computeRowMetrics. * handle properly the case where a position if not found on the row in both cursorX and getPosNearX (actually, this happens when selecting). * Some code cleanup and comments. --- 00README_STR_METRICS_BRANCH | 37 +++++++------ src/Row.cpp | 56 +++++++++++++------- src/Row.h | 9 +++- src/TextMetrics.cpp | 103 +++++++++++++++++------------------- 4 files changed, 112 insertions(+), 93 deletions(-) diff --git a/00README_STR_METRICS_BRANCH b/00README_STR_METRICS_BRANCH index 1bb70261c6..82026e895c 100644 --- a/00README_STR_METRICS_BRANCH +++ b/00README_STR_METRICS_BRANCH @@ -44,18 +44,16 @@ What is done: Next steps: * 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. + problems related to compose characters. Investigate whether RtL + strings could be drawn on a string-wise basis. -* 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. +* investigate whether strings could be cut at separators in RowPainter + 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 +* Get rid of old code in cursorX and getPosNearX; it has been kept for comparison purpose, guarded with KEEP_OLD_METRICS_CODE in order to check computations. @@ -67,33 +65,38 @@ 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. + elements with the same criteria. Currently 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 +* when clicking in the right margin, getPosNearX does not return the same value as before. I am not sure whether this is important. +* When selecting text, the display seems to move around. This is + because partly selected words are drawn in two parts, and in case + like "ef|fort" or "V|AN", there are some ligature or kerning effects + that change the display. I am not sure yet how to fix that. + Other differences in behavior (aka bug fixes): * end of paragraph markers metrics are computed with the font of the actual text, not default font. +* in RtL paragraphs, the end-of-paragraph marker does not move the row + to the right anymore (ticket #9040). + * When cursor is after a LtR separator just before a RtL chunk, the cursor position is computed better with the new code. diff --git a/src/Row.cpp b/src/Row.cpp index 6e60e97cf9..48b4fff521 100644 --- a/src/Row.cpp +++ b/src/Row.cpp @@ -63,18 +63,19 @@ double Row::Element::pos2x(pos_type const i) const pos_type Row::Element::x2pos(double &x, bool const low) const { //lyxerr << "x2pos: x=" << x << " w=" << width() << " " << *this; - // if element is rtl, flip x value + // If element is rtl, flip x value bool const rtl = font.isVisibleRightToLeft(); double x2 = rtl ? (width() - x) : x; - FontMetrics const & fm = theFontMetrics(font); double last_w = 0; double w = 0; size_t i = 0; - // non-STRING element only contain one position - if (type != STRING) { - w = width(); - } else { + switch (type) { + case VIRTUAL: + // those elements are actually empty (but they have a width) + break; + case STRING: { + FontMetrics const & fm = theFontMetrics(font); // FIXME: implement dichotomy search? for ( ; i < str.size() ; ++i) { last_w = w; @@ -82,8 +83,13 @@ pos_type Row::Element::x2pos(double &x, bool const low) const if (w > x2) break; } - // if (i == str.size()) - // lyxerr << " NOT FOUND "; + break; + } + case SEPARATOR: + case INSET: + case SPACE: + // those elements contain only one position + w = width(); } if (type == STRING && i == str.size()) @@ -91,24 +97,33 @@ pos_type Row::Element::x2pos(double &x, bool const low) const // round to the closest side. The !rtl is here to obtain the // same rounding as with the old code (this is cosmetic and // can be eventually removed). - else if (!low && (x2 - last_w + !rtl > w - x2)) { + else if (type != VIRTUAL && !low && (x2 - last_w + !rtl > w - x2)) { x2 = w; ++i; } else x2 = last_w; - // is element is rtl, flip values - if (rtl) { - x = width() - x2; - } else { - x = x2; - } + // is element is rtl, flip values back + x = rtl ? width() - x2 : x2; //lyxerr << "=> p=" << pos + i << " x=" << x << endl; return pos + i; } +pos_type Row::Element::left_pos() const +{ + return font.isVisibleRightToLeft() ? endpos : pos; +} + + +pos_type Row::Element::right_pos() const +{ + return font.isVisibleRightToLeft() ? pos : endpos; +} + + + Row::Row() : separator(0), label_hfill(0), x(0), right_margin(0), sel_beg(-1), sel_end(-1), @@ -197,21 +212,22 @@ ostream & operator<<(ostream & os, Row::Element const & e) switch (e.type) { case Row::STRING: - os << "STRING: `" << to_utf8(e.str) << "' " << e.dim.wid; + os << "STRING: `" << to_utf8(e.str) << "', "; break; case Row::VIRTUAL: - os << "VIRTUAL: `" << to_utf8(e.str) << "'"; + os << "VIRTUAL: `" << to_utf8(e.str) << "', "; break; case Row::INSET: - os << "INSET: " << to_utf8(e.inset->layoutName()); + os << "INSET: " << to_utf8(e.inset->layoutName()) << ", "; break; case Row::SEPARATOR: - os << "SEPARATOR: " << e.dim.wid << "+" << e.extra; + os << "SEPARATOR: extra=" << e.extra << ", "; break; case Row::SPACE: - os << "SPACE: " << e.dim.wid; + os << "SPACE: "; break; } + os << "width=" << e.width(); return os; } diff --git a/src/Row.h b/src/Row.h index 0a45aa0f1b..7725427b9b 100644 --- a/src/Row.h +++ b/src/Row.h @@ -66,7 +66,7 @@ public: // returns total width of element, including separator overhead double width() const { return dim.wid + extra; }; // returns position in pixels (from the left) of position - // \param i in the row element + // \param i in the row element. double pos2x(pos_type const i) const; /** Return character position that is the closest to @@ -77,6 +77,11 @@ public: */ pos_type x2pos(double &x, bool low = false) const; + // Returns the position on left side of the element. + pos_type left_pos() const; + // Returns the position on right side of the element. + pos_type right_pos() const; + // The kind of row element Type type; // position of the element in the paragraph @@ -200,7 +205,7 @@ public: /** * if row width is too large, remove all elements after last * separator and update endpos if necessary. If all that - * rename is a large word, cut it to \param width. + * remains is a large word, cut it to \param width. * \param body_pos minimum amount of text to keep. * \param width maximum width of the row */ diff --git a/src/TextMetrics.cpp b/src/TextMetrics.cpp index dd5b12c568..61a075818a 100644 --- a/src/TextMetrics.cpp +++ b/src/TextMetrics.cpp @@ -570,14 +570,10 @@ void TextMetrics::computeRowMetrics(pit_type const pit, Paragraph const & par = text_->getPar(pit); - double w = width - row.width(); + double w = width - row.right_margin - row.width(); // FIXME: put back this assertion when the crash on new doc is solved. //LASSERT(w >= 0, /**/); - //lyxerr << "\ndim_.wid " << dim_.wid << endl; - //lyxerr << "row.width() " << row.width() << endl; - //lyxerr << "w " << w << endl; - bool const is_rtl = text_->isRTL(par); if (is_rtl) row.x = rightMargin(pit); @@ -628,8 +624,6 @@ void TextMetrics::computeRowMetrics(pit_type const pit, && row.endpos() != par.size()) { setSeparatorWidth(row, w / ns); row.dimension().wid = width; - //lyxerr << "row.separator " << row.separator << endl; - //lyxerr << "ns " << ns << endl; } else if (is_rtl) { row.dimension().wid = width; row.x += w; @@ -637,11 +631,10 @@ void TextMetrics::computeRowMetrics(pit_type const pit, break; } case LYX_ALIGN_RIGHT: - row.dimension().wid = width; row.x += w; break; case LYX_ALIGN_CENTER: - row.dimension().wid += w / 2; + row.dimension().wid = width - w / 2; row.x += w / 2; break; } @@ -852,6 +845,15 @@ void TextMetrics::breakRow(Row & row, int const right_margin, pit_type const pit Inset const * ins = par.getInset(i); Dimension dim = pm.insetDimension(ins); row.add(i, ins, dim, *fi, par.lookupChange(i)); + } else if (c == ' ' && i + 1 == body_pos) { + // There is a space at i, but it should not be + // added as a separator, because it is just + // before body_pos. Instead, insert some spacing to + // align text + FontMetrics const & fm = theFontMetrics(text_->labelFont(par)); + int const add = max(fm.width(par.layout().labelsep), + labelEnd(pit) - row.width()); + row.addSpace(i, add, *fi, par.lookupChange(i)); } else if (par.isLineSeparator(i)) { // In theory, no inset has this property. If // this is done, a new addSeparator which @@ -865,15 +867,6 @@ void TextMetrics::breakRow(Row & row, int const right_margin, pit_type const pit else row.add(i, c, *fi, par.lookupChange(i)); - // end of paragraph marker - if (lyxrc.paragraph_markers - && i == end - 1 && size_type(pit + 1) < pars.size()) { - // enlarge the last character to hold the end-of-par marker - Font f(text_->layoutFont(pit)); - f.fontInfo().setColor(Color_paragraphmarker); - row.addVirtual(i + 1, docstring(1, char_type(0x00B6)), f, Change()); - } - // add inline completion width if (inlineCompletionLPos == i && !bv_->inlineCompletion().empty()) { @@ -900,21 +893,20 @@ void TextMetrics::breakRow(Row & row, int const right_margin, pit_type const pit ++i; ++fi; + } - // add the auto-hfill from label end to the body - if (body_pos && i == body_pos) { - FontMetrics const & fm = theFontMetrics(text_->labelFont(par)); - pos_type j = i; - if (!row.empty() - && row.back().type == Row::SEPARATOR) { - row.pop_back(); - --j; - } - int const add = max(fm.width(par.layout().labelsep), - labelEnd(pit) - row.width()); - row.addSpace(j, add, *fi, par.lookupChange(i)); - } - + // End of paragraph marker + if (lyxrc.paragraph_markers + && i == end && size_type(pit + 1) < pars.size()) { + // add a virtual element for the end-of-paragraph + // marker; it is shown on screen, but does not exist + // in the paragraph. + Font f(text_->layoutFont(pit)); + f.fontInfo().setColor(Color_paragraphmarker); + BufferParams const & bparams + = text_->inset().buffer().params(); + f.setLanguage(par.getParLanguage(bparams)); + row.addVirtual(end, docstring(1, char_type(0x00B6)), f, Change()); } row.finalizeLast(); @@ -930,8 +922,6 @@ void TextMetrics::breakRow(Row & row, int const right_margin, pit_type const pit // make sure that the RTL elements are in reverse ordering row.reverseRTL(text_->isRTL(par)); - - row.dimension().wid += right_margin; } @@ -1117,7 +1107,6 @@ void TextMetrics::setRowHeight(Row & row, pit_type const pit, pos_type TextMetrics::getPosNearX(pit_type const pit, Row const & row, int & x, bool & boundary) const { - /// For the main Text, it is possible that this pit is not /// yet in the CoordCache when moving cursor up. /// x Paragraph coordinate is always 0 for main text anyway. @@ -1131,13 +1120,11 @@ pos_type TextMetrics::getPosNearX(pit_type const pit, boundary = false; if (row.empty()) x = row.x; - else if (x < row.x) { - pos = row.front().font.isVisibleRightToLeft() ? - row.front().endpos : row.front().pos; + else if (x <= row.x) { + pos = row.front().left_pos(); x = row.x; - } else if (x > row.width() - row.right_margin) { - pos = row.back().font.isVisibleRightToLeft() ? - row.back().pos : row.back().endpos; + } else if (x >= row.width() - row.right_margin) { + pos = row.back().right_pos(); x = row.width() - row.right_margin; } else { double w = row.x; @@ -1152,13 +1139,14 @@ pos_type TextMetrics::getPosNearX(pit_type const pit, } w += cit->width(); } - if (cit == row.end()) - lyxerr << "NOT FOUND!! x=" << x - << ", wid=" << row.width() << endl; + if (cit == row.end()) { + pos = row.back().right_pos(); + x = row.width() - row.right_margin; + } /** This tests for the case where the cursor is placed * just before a font direction change. See comment on * the boundary_ member in DocIterator.h to understand - * how bounddary helps here. + * how boundary helps here. */ else if (pos == cit->endpos && cit + 1 != row.end() @@ -1618,28 +1606,35 @@ int TextMetrics::cursorX(CursorSlice const & sl, */ int const boundary_corr = (boundary && pos) ? -1 : 0; - //????? + /** Early return in trivial cases + * 1) the row is empty + * 2) the position is the left-most position of the row; there + * is a quirck herehowever: if the first element is virtual + * (end-of-par marker for example), then we have to look + * closer + */ if (row.empty() - || (row.begin()->font.isVisibleRightToLeft() - && pos == row.begin()->endpos)) + || (pos == row.begin()->left_pos() + && pos != row.begin()->right_pos())) return int(row.x); Row::const_iterator cit = row.begin(); double x = row.x; for ( ; cit != row.end() ; ++cit) { + /** Look whether the cursor is inside the element's + * span. Note that it is necessary to take the + * boundary in account, and to accept virtual + * elements, which have pos == endpos. + */ if (pos + boundary_corr >= cit->pos - && pos + boundary_corr < cit->endpos) { + && (pos + boundary_corr < cit->endpos + || cit->pos == cit->endpos)) { x += cit->pos2x(pos); break; } x += cit->width(); } - if (cit == row.end() - && (row.back().font.isVisibleRightToLeft() || pos != row.back().endpos)) - LYXERR0("cursorX(" << pos - boundary_corr - << ", " << boundary_corr << "): NOT FOUND! " << row); - #ifdef KEEP_OLD_METRICS_CODE pit_type const pit = sl.pit(); Paragraph const & par = text_->paragraphs()[pit]; -- 2.39.5