Juergen Spitzmueller wrote:
> Jean-Marc Lasgouttes wrote:
> > I'm very glad that you found the
> > cause, but the fix doesn't look
> > right.
>
> Too bad. Then I am at my witness end.
I have tried to enhance my witness a bit this year. Here's how I understand
this case:
After DEPM, doRecordUndo is finally called, where undo.end is calculated as
undo.end = cell.lastpit() - last_pit;
cell.lastpit() is the lastpit of the buffer _including_ the paragraph that
will be deleted. last_pit is the paragraph where the cursor sits.
If this is the first paragraph (pit = 0), then undo.end = cell.lastpit(),
which is 1 bigger than lastpit() of the buffer _after_ DEPM.
Now if undo is activated after DEPM, textUndoOrRedo calls doRecordUndo again,
with a last_pit that is calculated from cell_dit.lastpit() - undo.end. In our
case, this is -1, since, as elaborated, undo.end is 1 bigger than current
lastpit.
Until now, we thought that such a value is invalid, but as I understand it
now, this is correct, since last_pit is again only used to calculate
undo.end:
undo.end = cell.lastpit() - last_pit;
which is
undo.end = cell.lastpit() - (-1), e.g. undo.end = cell.lastpit() + 1
IMHO this is correct, since the deleted paragraph is being added again at this
stage, so undo.end has to increase by one. However, I'm not definitely sure.
Let's have a look at the crash. This is due to the first action of
doRecordUndo:
if (first_pit > last_pit)
std::swap(first_pit, last_pit);
this is of course triggeres if last_pit = -1, but it shouldn't in this case. I
don't know if it crashes because swap cannot handle negative values or
because of some later action, anyway, it just shouldn't try to swap the two
values here.
So my proposal to fix the bug now is
- if (first_pit > last_pit)
+ if (last_pit >= 0 && first_pit > last_pit)
std::swap(first_pit, last_pit);
(see attached patch).
As expected, this fixes the crash and also works as expected for undo after
DEPM.
Does this sound reasonable?
Jürgen
Index: undo.C
===================================================================
RCS file: /usr/local/lyx/cvsroot/lyx-devel/src/undo.C,v
retrieving revision 1.69
diff -u -r1.69 undo.C
--- undo.C 24 Nov 2005 16:22:39 -0000 1.69
+++ undo.C 2 Jan 2006 11:43:19 -0000
@@ -64,7 +64,9 @@
bool isFullBuffer,
limited_stack<Undo> & stack)
{
- if (first_pit > last_pit)
+ // last_pit can legally be -1 when a DEPM action in the first paragraph
+ // has to be restored! Then undo.end below is cell.lastpit + 1
+ if (last_pit >= 0 && first_pit > last_pit)
std::swap(first_pit, last_pit);
// create the position information of the Undo entry
Undo undo;
@@ -150,10 +152,9 @@
Buffer * buf = bv.buffer();
DocIterator cell_dit = undo.cell.asDocIterator(&buf->inset());
- doRecordUndo(Undo::ATOMIC, cell_dit,
- undo.from, cell_dit.lastpit() - undo.end, bv.cursor(),
- undo.bparams, undo.isFullBuffer,
- otherstack);
+ doRecordUndo(Undo::ATOMIC, cell_dit, undo.from,
+ cell_dit.lastpit() - undo.end, bv.cursor(),
+ undo.bparams, undo.isFullBuffer, otherstack);
// This does the actual undo/redo.
//lyxerr << "undo, performing: " << undo << std::endl;
