Re: [patch] bug 2155: Crash when undoing DEPM deletion of the first par (pit = 0)
Juergen == Juergen Spitzmueller [EMAIL PROTECTED] writes:
Juergen I have tried to enhance my witness a bit this year. Here's
Juergen how I understand this case:
I think the problem is in DEPM: since it deletes a paragraph, the
recordUndo call should span two paragraphs (think about deleting a
selection that spans more than a paragraph).
I propose the following patch. What do you think?
JMarc
Index: src/ChangeLog
===
RCS file: /usr/local/lyx/cvsroot/lyx-devel/src/ChangeLog,v
retrieving revision 1.2351
diff -u -p -r1.2351 ChangeLog
--- src/ChangeLog 1 Jan 2006 23:06:22 - 1.2351
+++ src/ChangeLog 4 Jan 2006 11:36:04 -
@@ -1,3 +1,8 @@
+2006-01-04 Jean-Marc Lasgouttes [EMAIL PROTECTED]
+
+ * text2.C (deleteEmptyParagraphMechanism): since we delete a
+ paragraph, recordUndo should span two paragraphs (bug 2155).
+
2006-01-01 Martin Vermeer [EMAIL PROTECTED]
* rowpainter.C (paintPar): always repaint the row with the
Index: src/text2.C
===
RCS file: /usr/local/lyx/cvsroot/lyx-devel/src/text2.C,v
retrieving revision 1.636
diff -u -p -r1.636 text2.C
--- src/text2.C 6 Dec 2005 14:54:23 - 1.636
+++ src/text2.C 4 Jan 2006 11:36:04 -
@@ -1234,7 +1234,8 @@ bool LyXText::deleteEmptyParagraphMechan
if (oldpar.empty() || (oldpar.size() == 1 oldpar.isLineSeparator(0))) {
// Delete old par.
- recordUndo(old, Undo::ATOMIC, old.pit());
+ recordUndo(old, Undo::ATOMIC,
+ old.pit(), min(old.pit() + 1, old.lastpit()));
ParagraphList plist = old.text()-paragraphs();
plist.erase(plist.begin() + old.pit());
Re: [patch] bug 2155: Crash when undoing DEPM deletion of the first par (pit = 0)
Jean-Marc Lasgouttes wrote: I think the problem is in DEPM: since it deletes a paragraph, the recordUndo call should span two paragraphs (think about deleting a selection that spans more than a paragraph). I propose the following patch. What do you think? I think you are right. I wonder why I did no see this. Jürgen
Re: [patch] bug 2155: Crash when undoing DEPM deletion of the first par (pit = 0)
Juergen == Juergen Spitzmueller [EMAIL PROTECTED] writes: Juergen Jean-Marc Lasgouttes wrote: I think the problem is in DEPM: since it deletes a paragraph, the recordUndo call should span two paragraphs (think about deleting a selection that spans more than a paragraph). I propose the following patch. What do you think? I had this nagging feeling that something had to be wrong in dEPM... That's why I was not satisfied with your patches. Juergen I think you are right. I wonder why I did no see this. Can you test it a little bit? I may not have thought about everything (for example the min was added after I tried to delete the last line of a paragraph with backspace). JMarc
Re: [patch] bug 2155: Crash when undoing DEPM deletion of the first par (pit = 0)
Jean-Marc Lasgouttes wrote: Can you test it a little bit? I may not have thought about everything (for example the min was added after I tried to delete the last line of a paragraph with backspace). I do not have much time for testing ATM. I tested the cases that I also tested with my patches, and they all work as expected. I'd propose to commit your patch. Jürgen
Re: [patch] bug 2155: Crash when undoing DEPM deletion of the first par (pit = 0)
Juergen == Juergen Spitzmueller [EMAIL PROTECTED] writes: Juergen Jean-Marc Lasgouttes wrote: Can you test it a little bit? I may not have thought about everything (for example the min was added after I tried to delete the last line of a paragraph with backspace). Juergen I do not have much time for testing ATM. I tested the cases Juergen that I also tested with my patches, and they all work as Juergen expected. I'd propose to commit your patch. OK, I applied it. JMarc
Re: [patch] bug 2155: Crash when undoing DEPM deletion of the first par (pit = 0)
> "Juergen" == Juergen Spitzmueller <[EMAIL PROTECTED]> writes:
Juergen> I have tried to enhance my witness a bit this year. Here's
Juergen> how I understand this case:
I think the problem is in DEPM: since it deletes a paragraph, the
recordUndo call should span two paragraphs (think about deleting a
selection that spans more than a paragraph).
I propose the following patch. What do you think?
JMarc
Index: src/ChangeLog
===
RCS file: /usr/local/lyx/cvsroot/lyx-devel/src/ChangeLog,v
retrieving revision 1.2351
diff -u -p -r1.2351 ChangeLog
--- src/ChangeLog 1 Jan 2006 23:06:22 - 1.2351
+++ src/ChangeLog 4 Jan 2006 11:36:04 -
@@ -1,3 +1,8 @@
+2006-01-04 Jean-Marc Lasgouttes <[EMAIL PROTECTED]>
+
+ * text2.C (deleteEmptyParagraphMechanism): since we delete a
+ paragraph, recordUndo should span two paragraphs (bug 2155).
+
2006-01-01 Martin Vermeer <[EMAIL PROTECTED]>
* rowpainter.C (paintPar): always repaint the row with the
Index: src/text2.C
===
RCS file: /usr/local/lyx/cvsroot/lyx-devel/src/text2.C,v
retrieving revision 1.636
diff -u -p -r1.636 text2.C
--- src/text2.C 6 Dec 2005 14:54:23 - 1.636
+++ src/text2.C 4 Jan 2006 11:36:04 -
@@ -1234,7 +1234,8 @@ bool LyXText::deleteEmptyParagraphMechan
if (oldpar.empty() || (oldpar.size() == 1 && oldpar.isLineSeparator(0))) {
// Delete old par.
- recordUndo(old, Undo::ATOMIC, old.pit());
+ recordUndo(old, Undo::ATOMIC,
+ old.pit(), min(old.pit() + 1, old.lastpit()));
ParagraphList & plist = old.text()->paragraphs();
plist.erase(plist.begin() + old.pit());
Re: [patch] bug 2155: Crash when undoing DEPM deletion of the first par (pit = 0)
Jean-Marc Lasgouttes wrote: > I think the problem is in DEPM: since it deletes a paragraph, the > recordUndo call should span two paragraphs (think about deleting a > selection that spans more than a paragraph). > > I propose the following patch. What do you think? I think you are right. I wonder why I did no see this. Jürgen
Re: [patch] bug 2155: Crash when undoing DEPM deletion of the first par (pit = 0)
> "Juergen" == Juergen Spitzmueller <[EMAIL PROTECTED]> writes: Juergen> Jean-Marc Lasgouttes wrote: >> I think the problem is in DEPM: since it deletes a paragraph, the >> recordUndo call should span two paragraphs (think about deleting a >> selection that spans more than a paragraph). >> >> I propose the following patch. What do you think? I had this nagging feeling that something had to be wrong in dEPM... That's why I was not satisfied with your patches. Juergen> I think you are right. I wonder why I did no see this. Can you test it a little bit? I may not have thought about everything (for example the min was added after I tried to delete the last line of a paragraph with backspace). JMarc
Re: [patch] bug 2155: Crash when undoing DEPM deletion of the first par (pit = 0)
Jean-Marc Lasgouttes wrote: > Can you test it a little bit? I may not have thought about everything > (for example the min was added after I tried to delete the last line > of a paragraph with backspace). I do not have much time for testing ATM. I tested the cases that I also tested with my patches, and they all work as expected. I'd propose to commit your patch. Jürgen
Re: [patch] bug 2155: Crash when undoing DEPM deletion of the first par (pit = 0)
> "Juergen" == Juergen Spitzmueller <[EMAIL PROTECTED]> writes: Juergen> Jean-Marc Lasgouttes wrote: >> Can you test it a little bit? I may not have thought about >> everything (for example the min was added after I tried to delete >> the last line of a paragraph with backspace). Juergen> I do not have much time for testing ATM. I tested the cases Juergen> that I also tested with my patches, and they all work as Juergen> expected. I'd propose to commit your patch. OK, I applied it. JMarc
Re: [patch] bug 2155: Crash when undoing DEPM deletion of the first par (pit = 0)
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 - 1.69
+++ undo.C 2 Jan 2006 11:43:19 -
@@ -64,7 +64,9 @@
bool isFullBuffer,
limited_stackUndo 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;
Re: [patch] bug 2155: Crash when undoing DEPM deletion of the first par (pit = 0)
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 - 1.69
+++ undo.C 2 Jan 2006 11:43:19 -
@@ -64,7 +64,9 @@
bool isFullBuffer,
limited_stack & 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(>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;
[patch] bug 2155: Crash when undoing DEPM deletion of the first par (pit = 0)
http://bugzilla.lyx.org/show_bug.cgi?id=2155 The attached patch fixes a major bug (undo crash). See the verbose discussion on bugzilla for details. Comments? 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 - 1.69 +++ undo.C 31 Dec 2005 14:49:08 - @@ -150,10 +150,14 @@ 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); + pit_type end = cell_dit.lastpit() - undo.end; + // this might happen if DEPM has deleted a paragraph with pit = 0 + // (see bug 2155) + if (end 0) + end = 0; + + doRecordUndo(Undo::ATOMIC, cell_dit, undo.from, end, bv.cursor(), + undo.bparams, undo.isFullBuffer, otherstack); // This does the actual undo/redo. //lyxerr undo, performing: undo std::endl;
Re: [patch] bug 2155: Crash when undoing DEPM deletion of the first par (pit = 0)
I'm very glad that you found the cause, but the fix doesn't look right. JMarc
Re: [patch] bug 2155: Crash when undoing DEPM deletion of the first par (pit = 0)
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. Leaving now to welcome the new year. Have a nice party everybody. Jürgen
[patch] bug 2155: Crash when undoing DEPM deletion of the first par (pit = 0)
http://bugzilla.lyx.org/show_bug.cgi?id=2155 The attached patch fixes a major bug (undo crash). See the verbose discussion on bugzilla for details. Comments? 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 - 1.69 +++ undo.C 31 Dec 2005 14:49:08 - @@ -150,10 +150,14 @@ Buffer * buf = bv.buffer(); DocIterator cell_dit = undo.cell.asDocIterator(>inset()); - doRecordUndo(Undo::ATOMIC, cell_dit, - undo.from, cell_dit.lastpit() - undo.end, bv.cursor(), - undo.bparams, undo.isFullBuffer, - otherstack); + pit_type end = cell_dit.lastpit() - undo.end; + // this might happen if DEPM has deleted a paragraph with pit = 0 + // (see bug 2155) + if (end < 0) + end = 0; + + doRecordUndo(Undo::ATOMIC, cell_dit, undo.from, end, bv.cursor(), + undo.bparams, undo.isFullBuffer, otherstack); // This does the actual undo/redo. //lyxerr << "undo, performing: " << undo << std::endl;
Re: [patch] bug 2155: Crash when undoing DEPM deletion of the first par (pit = 0)
I'm very glad that you found the cause, but the fix doesn't look right. JMarc
Re: [patch] bug 2155: Crash when undoing DEPM deletion of the first par (pit = 0)
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. Leaving now to welcome the new year. Have a nice party everybody. Jürgen
