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

@@ -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
+  problems related to compose characters. Investigate whether RtL
-  needed in FontMetrics::width.
+  strings could be drawn on a string-wise basis.
 
-* investigate whether RtL strings could be drawn on a string-wise basis.
+* investigate whether strings could be cut at separators in RowPainter
-
+  only in justified text. This would speed-up painting in other cases
-* investigate whether Row::SEPARATOR elements could be used only in
+  by lowering the number of strings to draw.
-  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
+  elements with the same criteria. Currently breakRow does not
-  does not consider on-the-fly spell-checking and selection changes,
+  consider on-the-fly spell-checking and selection changes, but it is
-  but it is not clear to me that it is required. Moreover, this thing
+  not clear to me that it is required. Moreover, this thing would only
-  would only work if we are sure that the Row object is up-to-date
+  work if we are sure that the Row object is up-to-date when drawing
-  when drawing happens. This depends on the update machinery.
+  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.
 

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
+	switch (type) {
-	if (type != STRING) {
+	case VIRTUAL:
-		w = width();
+		// those elements are actually empty (but they have a width)
-	} else {
+		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())
+		break;
-		// 	lyxerr << " NOT FOUND ";
+	}
+	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
+	// is element is rtl, flip values back
-	if (rtl) {
+	x = rtl ? width() - x2 : x2;
-		x = width() - x2;
-	} else {
-		x = 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;
 }
 

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
 	 */

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
+	// End of paragraph marker
-		if (body_pos && i == body_pos) {
+	if (lyxrc.paragraph_markers
-			FontMetrics const & fm = theFontMetrics(text_->labelFont(par));
+	    && i == end && size_type(pit + 1) < pars.size()) {
-			pos_type j = i;
+		// add a virtual element for the end-of-paragraph
-			if (!row.empty()
+		// marker; it is shown on screen, but does not exist
-			    && row.back().type == Row::SEPARATOR) {
+		// in the paragraph.
-				row.pop_back();
+		Font f(text_->layoutFont(pit));
-				--j;
+		f.fontInfo().setColor(Color_paragraphmarker);
-			}
+		BufferParams const & bparams
-			int const add = max(fm.width(par.layout().labelsep),
+			= text_->inset().buffer().params();
-					    labelEnd(pit) - row.width());
+		f.setLanguage(par.getParLanguage(bparams));
-			row.addSpace(j, add, *fi, par.lookupChange(i));
+		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) {
+	else if (x <= row.x) {
-		pos = row.front().font.isVisibleRightToLeft() ?
+		pos = row.front().left_pos();
-			row.front().endpos : row.front().pos;
 		x = row.x;
-	} else if (x > row.width() - row.right_margin) {
+	} else if (x >= row.width() - row.right_margin) {
-		pos = row.back().font.isVisibleRightToLeft() ?
+		pos = row.back().right_pos();
-			row.back().pos : row.back().endpos;
 		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())
+		if (cit == row.end()) {
-			lyxerr << "NOT FOUND!! x=" << x
+			pos = row.back().right_pos();
-			       << ", wid=" << row.width() << endl;
+			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()->left_pos()
-		&& pos == row.begin()->endpos))
+		&& 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];