Alfredo Braunstein
2014-10-13 12:29:06 UTC
The problem was indeed correctly identified by JM in changeset 7c3d1d7:
"The problem is the use of cursor movement methods to update cursor.
Cursor::forwardPos() steps into insets, which is not always what we
want. The problem here is that there is a math inset just after the
accepted change, and that the cursor steps into it for some reason.
This code is a nightmare anyway."
The fix however seems bogus to me:
bool findChange(DocIterator & cur, bool next)
{
if (!next)
cur.backwardPos();
for (; cur; next ? cur.forwardPos() : cur.backwardPos())
// if we search backwards, take a step forward
// to correctly set the anchor
- cur.forwardPos();
+ cur.top().forwardPos();
return true;
}
}
If you look at the top of the chunk, there is another
forwardPos/backwardPos that could step into an inset (and it is the
one iterated by the way). Besides, stepping into insets is needed when
looking for changes in other cases (accept all etc). Thus IMHO 7c3d1d7
should be reverted.
I think that we should first search the initial pos of the change in a
"flat" version of findNextChange as in the attachment. Note that at
the end I still call findNextChange, but just to select the full range
(a bit overkill).
I suspect that the code could be simplified enormously by adding an
acessor function to Paragraph that returns the range of the change a
given pos is in. But I'm not going to do it now...
Comments? (specially JM)
A/
"The problem is the use of cursor movement methods to update cursor.
Cursor::forwardPos() steps into insets, which is not always what we
want. The problem here is that there is a math inset just after the
accepted change, and that the cursor steps into it for some reason.
This code is a nightmare anyway."
The fix however seems bogus to me:
bool findChange(DocIterator & cur, bool next)
{
if (!next)
cur.backwardPos();
for (; cur; next ? cur.forwardPos() : cur.backwardPos())
// if we search backwards, take a step forward
// to correctly set the anchor
- cur.forwardPos();
+ cur.top().forwardPos();
return true;
}
}
If you look at the top of the chunk, there is another
forwardPos/backwardPos that could step into an inset (and it is the
one iterated by the way). Besides, stepping into insets is needed when
looking for changes in other cases (accept all etc). Thus IMHO 7c3d1d7
should be reverted.
I think that we should first search the initial pos of the change in a
"flat" version of findNextChange as in the attachment. Note that at
the end I still call findNextChange, but just to select the full range
(a bit overkill).
I suspect that the code could be simplified enormously by adding an
acessor function to Paragraph that returns the range of the change a
given pos is in. But I'm not going to do it now...
Comments? (specially JM)
A/