11/09/2013 11:43, Hashini Senaratne:
As &row do not give the expected row address in checkCursorLeftEdge() method
in BufferView when selecting certain positions in Math inset using the mouse
pointer, above first two bugs caused. Here the row is defined as,
Row const & row = cur.bottomRow();
At this situation the wideRow is differs from current_row_ in setCurrentRow
method in Cursor.cpp. As we call that method as,
// Set the row on which the cursor lives.
cur.setCurrentRow(&row);
Here the left_edge_ gets set to zero (0).
I think this behavior (of Cursor::bottomRow) is fixed in the attached
commit. I also reverted some of the workarounds that you introduced.
Please test and commit if it is satisfying.
If you really want to know what was wrong, it is a matter of cursor
boundary. What's that? DocIterator.h says:
/**
* Normally, when the cursor is at position i, it is painted *before*
* the character at position i. However, what if we want the cursor
* painted *after* position i? That's what boundary_ is for: if
* boundary_==true, the cursor is painted *after* position i-1, instead
* of before position i.
What does this matter? The example in changesNeeded.lyx contains a
displayed formula, which means that the position at the end of
"Math-formula" is logically the same as the one just in fron of the
equation. The only different is that you end up at one or the other
depending on where you come from. This should not cocnern us, excpet
that Cursor::bottomRow sometimes got the value of the boundary boolean
false, and therefore pointed to the wrong row.
I have tried to remove unneeded workarounds in my patch, but there may
be more. In particular I did not touch the early return in
checkLeftEdge. Is it really needed?
The major thing I am suffering now is the behaviour of the vertical scroll
bar. I doubt, whether a code segment of this project, created the abnormal
behaviour. Try scrolling down using vertical scroll bar in changesNeeded.lyx
Yes, I can see it now and it was not present in master. Therefore there
is something wrong, but I cannot tell you what.
Could you please advice me on the next steps. Is the current implementation
is good enough to move to the next stage (GUI based scrolling).
There are some remaining points:
1/ you shall get rid of special handling for tables and check that
everything still works.
2/ The code for target_x should take left edge in account (it should
probably reflect the actual position of the cursor on screen. Therefore,
when going Up from a scrolled row, the cursor would go to the character
physically above.
I am not 100% sure that this behavior would be better, but I feel uneasy
with the current one.
3/ in changesNeeded.lyx, If you set the cursor at the left of
"Graphics", then move cursor to the left, so that the long formula is
completely scrolled, and finally use CursorUp, the equation is not
repainted with left_edge==0. I think this is because of the early return
that you introduced.
JMarc
>From 27c84104b3a3c2a1aeb04765444f902d1230173c Mon Sep 17 00:00:00 2001
From: Jean-Marc Lasgouttes <[email protected]>
Date: Wed, 11 Sep 2013 14:13:26 +0200
Subject: [PATCH] Fix subtle bug in Cursor::bottomRow
The method used to pass boundary() to getRow when cursor was in a nested inset. This is wrong and created subtle bugs in the code.
Cursor::textRow is modified to use the same code, although this should not change anything in this case.
Cursor::setCurrentRow is therefore updated to remove an extra test.
Some unrelated changes:
- rename row_need_slde to row_moved
- some small code simplifications
---
src/BufferView.cpp | 13 ++++++-------
src/Cursor.cpp | 9 +++------
2 files changed, 9 insertions(+), 13 deletions(-)
diff --git a/src/BufferView.cpp b/src/BufferView.cpp
index e866e8e..de3931c 100644
--- a/src/BufferView.cpp
+++ b/src/BufferView.cpp
@@ -2847,7 +2847,7 @@ void checkCursorLeftEdge(PainterInfo & pi, Cursor const & cur,
Bidi bidi;
Row const & row = cur.bottomRow();
BufferView const & bv = cur.bv();
- bool row_need_slide = false;
+ bool row_moved = false;
// Left edge value of the screen in pixels
int left_edge = cur.getLeftEdge();
@@ -2876,19 +2876,18 @@ void checkCursorLeftEdge(PainterInfo & pi, Cursor const & cur,
// If need to slide right
if (cur_x < left_edge + 10) {
left_edge = cur_x - 10;
- row_need_slide = true;
+ row_moved = true;
}
// If need to slide left ()
else if (cur_x > left_edge + bv.workWidth() - 10) {
left_edge = cur_x - bv.workWidth() + 10;
- row_need_slide = true;
+ row_moved = true;
}
- if (cur.getCurrentRow() != cur.getPreviousRow()
- && strategy == NoScreenUpdate
- && row_need_slide) {
- ScreenUpdateStrategy const oldstrat = strategy;
+ if (strategy == NoScreenUpdate && row_moved) {
+ // FIXME: if one uses SingleParUpdate, then home/end
+ // will not work on long rows. Why?
strategy = FullScreenUpdate;
}
diff --git a/src/Cursor.cpp b/src/Cursor.cpp
index 316ed02..2c537d3 100644
--- a/src/Cursor.cpp
+++ b/src/Cursor.cpp
@@ -538,8 +538,7 @@ Row const & Cursor::textRow() const
{
CursorSlice const & cs = innerTextSlice();
ParagraphMetrics const & pm = bv().parMetrics(cs.text(), cs.pit());
- bool const bndry = inTexted() ? boundary() : false;
- return pm.getRow(cs.pos(), bndry);
+ return pm.getRow(cs.pos(), boundary() && cs == top());
}
@@ -547,8 +546,7 @@ Row const & Cursor::bottomRow() const
{
CursorSlice const & cs = bottom();
ParagraphMetrics const & pm = bv().parMetrics(cs.text(), cs.pit());
- bool const bndry = inTexted() ? boundary() : false;
- return pm.getRow(cs.pos(), bndry);
+ return pm.getRow(cs.pos(), boundary() && cs == top());
}
@@ -592,8 +590,7 @@ void Cursor::setCurrentRow(Row const * wideRow) const
previous_row_ = 0;
// Since we changed row, the scroll offset is not valid anymore
- if (!selectionEnd().inMathed())
- left_edge_ = 0;
+ left_edge_ = 0;
current_row_ = wideRow;
}
--
1.7.0.4
