Cleanup Page Up/Down code

Since BufferView::scroll() does not correct scrolling amount at
top/bottom anymore, remove its return value and add a new one to
BufferView::updateMetrics(bool) that returns this correction.

Rely on updateMetrics(false) to set the metrics straight after moving
the anchor y position.

Use assert_in_view = false when calling TextMetrics::editXY() because
this parameter does not work as advertised: if an inset not totally
visible, the code will not try to go inside it to look for a smaller
row that is totally visible.

Two small (hopfully useful) modifications:
- take into account the cursor X target instead of its screen position
- adapt the cursor position depending on
  lyxrc.mac_like_cursor_movement when hitting start or end of
  document.

src/BufferView.cpp

@@ -2128,6 +2128,8 @@ void BufferView::dispatch(FuncRequest const & cmd, DispatchResult & dr)
 	case LFUN_SCREEN_UP:
 	case LFUN_SCREEN_DOWN: {
 		Point p = getPos(cur);
+		// replace x by the cursor target
+		p.x = cur.targetX();
 		// This code has been commented out to enable to scroll down a
 		// document, even if there are large insets in it (see bug #5465).
 		/*if (p.y < 0 || p.y > height_) {
@@ -2135,29 +2137,28 @@ void BufferView::dispatch(FuncRequest const & cmd, DispatchResult & dr)
 			showCursor();
 			p = getPos(cur);
 		}*/
-		int const scrolled = scroll(act == LFUN_SCREEN_UP
-			? -height_ : height_);
-		if (act == LFUN_SCREEN_UP && scrolled > -height_)
-			p = Point(0, 0);
-		if (act == LFUN_SCREEN_DOWN && scrolled < height_)
-			p = Point(width_, height_);
+		scroll(act == LFUN_SCREEN_UP ? -height_ : height_);
+		// update metrics
+		int const correction = updateMetrics(false);
+		if (act == LFUN_SCREEN_UP && correction < 0)
+			p = Point(lyxrc.mac_like_cursor_movement ? 0 : p.x, 0);
+		if (act == LFUN_SCREEN_DOWN && correction > 0)
+			p = Point(lyxrc.mac_like_cursor_movement ? width_ : p.x, height_);
 		bool const in_texted = cur.inTexted();
 		cur.setCursor(doc_iterator_begin(cur.buffer()));
 		cur.selHandle(false);
-		// Force an immediate computation of metrics because we need it below
-		if (scrolled)
-			processUpdateFlags(Update::Force);
 
 		d->text_metrics_[&buffer_.text()].editXY(cur, p.x, p.y,
-			true, act == LFUN_SCREEN_UP);
+			false, act == LFUN_SCREEN_UP);
 		//FIXME: what to do with cur.x_target()?
 		bool update = in_texted && cur.bv().checkDepm(cur, old);
 		cur.finishUndo();
 
-		if (update || cur.mark())
+		if (update) {
 			dr.screenUpdate(Update::Force | Update::FitCursor);
-		if (update)
 			dr.forceBufferUpdate();
+		} else
+			dr.screenUpdate(Update::ForceDraw | Update::FitCursor);
 		break;
 	}
 
@@ -2168,18 +2169,24 @@ void BufferView::dispatch(FuncRequest const & cmd, DispatchResult & dr)
 			scroll_step = d->scrollbarParameters_.single_step;
 		else if (scroll_type == "page")
 			scroll_step = d->scrollbarParameters_.page_step;
-		else
-			return;
-		string const scroll_quantity = cmd.getArg(1);
-		if (scroll_quantity == "up")
-			scrollUp(scroll_step);
-		else if (scroll_quantity == "down")
-			scrollDown(scroll_step);
 		else {
-			int const scroll_value = convert<int>(scroll_quantity);
-			if (scroll_value)
-				scroll(scroll_step * scroll_value);
+			dispatched = false;
+			return;
 		}
+
+		string const scroll_quantity = cmd.getArg(1);
+
+		if (scroll_quantity == "up")
+			scroll(-scroll_step);
+		else if (scroll_quantity == "down")
+			scroll(scroll_step);
+		else if (isStrInt(scroll_quantity))
+			scroll(scroll_step * convert<int>(scroll_quantity));
+		else {
+			dispatched = false;
+			return;
+		}
+
 		dr.screenUpdate(Update::ForceDraw);
 		dr.forceBufferUpdate();
 		break;
@@ -2852,24 +2859,9 @@ int BufferView::minVisiblePart()
 }
 
 
-int BufferView::scroll(int pixels)
+void BufferView::scroll(int pixels)
 {
 	d->anchor_ypos_ -= pixels;
-	return -pixels;
-}
-
-
-int BufferView::scrollDown(int pixels)
-{
-	d->anchor_ypos_ -= pixels;
-	return -pixels;
-}
-
-
-int BufferView::scrollUp(int pixels)
-{
-	d->anchor_ypos_ += pixels;
-	return pixels;
 }
 
 
@@ -3228,10 +3220,10 @@ void BufferView::updateMetrics()
 }
 
 
-void BufferView::updateMetrics(bool force)
+int BufferView::updateMetrics(bool force)
 {
 	if (!ready())
-		return;
+		return 0;
 
 	//LYXERR0("updateMetrics " << _v_(force));
 
@@ -3264,6 +3256,7 @@ void BufferView::updateMetrics(bool force)
 	tm.updateMetrics(d->anchor_pit_, d->anchor_ypos_, height_);
 
 	// Check that the end of the document is not too high
+	int const old_ypos = d->anchor_ypos_;
 	int const min_visible = lyxrc.scroll_below_document ? minVisiblePart() : height_;
 	if (tm.last().first == lastpit && tm.last().second->hasPosition()
 	     && tm.last().second->bottom() < min_visible) {
@@ -3280,6 +3273,9 @@ void BufferView::updateMetrics(bool force)
 		tm.updateMetrics(d->anchor_pit_, d->anchor_ypos_, height_);
 	}
 
+	// The global adjustment that have been made above
+	int const correction = d->anchor_ypos_ - old_ypos;
+
 	/* FIXME: do we want that? It avoids potential issues with old
 	 * paragraphs that should have been recomputed but have not, at
 	 * the price of potential extra metrics computation. I do not
@@ -3315,6 +3311,7 @@ void BufferView::updateMetrics(bool force)
 		LYXERR(Debug::WORKAREA, "BufferView::updateMetrics");
 		d->coord_cache_.dump();
 	}
+	return correction;
 }
 
 

src/BufferView.h

@@ -218,18 +218,18 @@ public:
 	/// Ensure the passed cursor \p dit is visible.
 	/// This method will automatically scroll and update the BufferView
 	/// (metrics+drawing) if needed.
-	/// \param how Use this scroll strategy
+	/// \param how: where the cursor should appear (visible/top/center)
 	void showCursor(DocIterator const & dit, ScrollType how);
 	/// Scroll to the cursor.
-	/// \param how Use this scroll strategy
+	/// This only updates the anchor vertical position, but does not
+	/// recompute metrics nor trigger a screen refresh.
+	/// \param how: where the cursor should appear (visible/top/center)
 	/// \return true if screen was scrolled
 	bool scrollToCursor(DocIterator const & dit, ScrollType how);
-	/// scroll down document by the given number of pixels.
-	int scrollDown(int pixels);
-	/// scroll up document by the given number of pixels.
-	int scrollUp(int pixels);
-	/// scroll document by the given number of pixels.
-	int scroll(int pixels);
+	/// scroll the view by the given number of pixels. This only
+	/// updates the anchor vertical position, but does not recompute
+	/// metrics nor trigger a screen refresh.
+	void scroll(int pixels);
 	/// Scroll the view by a number of pixels.
 	void scrollDocView(int pixels);
 	/// Set the cursor position based on the scrollbar one.
@@ -418,15 +418,15 @@ private:
 	/// Update current paragraph metrics.
 	/// \return true if no further update is needed.
 	bool singleParUpdate();
-	/** Helper for the public updateMetrics() and for processUpdateFlags()
-	 * * When \c force is true, get rid of all paragraph metrics and
-         rebuild them anew.
-	 * * When it is false, keep the paragraphs that are still visible in
-	 *   WorkArea and rebuild the missing ones.
-	 *
-	 * This does also set the anchor paragraph and its position correctly
+	/** Helper for the public updateMetrics() and for processUpdateFlags().
+	 * This does also set the anchor paragraph and its position correctly.
+	 * \param force when true, get rid of all paragraph metrics and
+	 *    rebuild them anew. Otherwise keep the paragraphs that are
+	 *    still visible in work area and rebuild the missing ones.
+	 * \return the correction needed (same sign as anchor vertical
+	 * position change) when hitting top or bottom constraints.
 	*/
-	void updateMetrics(bool force);
+	int updateMetrics(bool force);
 
 	// Set the row on which the cursor lives.
 	void setCurrentRowSlice(CursorSlice const & rowSlice);