[patch] fix bug 3886: Cut/paste bug with environment depths in 1.5svn
The problem has nothing to do with env depths, just with cut/past of empty paragraphs. The problem is that Text::acceptChanges on the internal cut/paste paragraph list calls DEPM and this removes an empty paragraph at the beginning. The fix is to move the bulk of acceptChanges to paragraph_funcs (where it belongs IMO) and the version in Text just calls that and then DEPM, while in cap we just call the plain version. I think this is the right thing to do, but on a first test this uncovers a DEPM bug :-O ... A/
Re: [patch] fix bug 3886: Cut/paste bug with environment depths in 1.5svn
> "Alfredo" == Alfredo Braunstein <[EMAIL PROTECTED]> writes: Alfredo> The problem has nothing to do with env depths, just with Alfredo> cut/past of empty paragraphs. The problem is that Alfredo> Text::acceptChanges on the internal cut/paste paragraph list Alfredo> calls DEPM and this removes an empty paragraph at the Alfredo> beginning. Alfredo> The fix is to move the bulk of acceptChanges to Alfredo> paragraph_funcs (where it belongs IMO) and the version in Alfredo> Text just calls that and then DEPM, while in cap we just call Alfredo> the plain version. Alfredo> I think this is the right thing to do, but on a first test Alfredo> this uncovers a DEPM bug :-O ... So now we have patch in one place and the explanation in another place... http://bugzilla.lyx.org/attachment.cgi?id=1927&action=view JMarc
Re: [patch] fix bug 3886: Cut/paste bug with environment depths in 1.5svn
Alfredo Braunstein wrote:
> The problem has nothing to do with env depths, just with cut/past of empty
> paragraphs. The problem is that Text::acceptChanges on the internal
> cut/paste paragraph list calls DEPM and this removes an empty paragraph at
> the beginning.
>
> The fix is to move the bulk of acceptChanges to paragraph_funcs (where it
> belongs IMO) and the version in Text just calls that and then DEPM, while
> in cap we just call the plain version.
... and the patch.
> I think this is the right thing to do, but on a first test this uncovers a
> DEPM bug :-O ...
Seems DEPM was not called after insertion, the following change to
pasteParagraphList (included in the patch) seems to fix it:
- text->setCursor(cur.top(), ppp.first, ppp.second);
+ text->setCursor(cur, ppp.first, ppp.second);
A/
Index: Text.cpp
===
--- Text.cpp (revision 18885)
+++ Text.cpp (working copy)
@@ -992,36 +992,9 @@
void Text::acceptChanges(BufferParams const & bparams)
{
- pit_type pars_size = static_cast(pars_.size());
-
- // first, accept changes within each individual paragraph
- // (do not consider end-of-par)
- for (pit_type pit = 0; pit < pars_size; ++pit) {
- if (!pars_[pit].empty()) // prevent assertion failure
- pars_[pit].acceptChanges(bparams, 0, pars_[pit].size());
- }
-
- // next, accept imaginary end-of-par characters
- for (pit_type pit = 0; pit < pars_size; ++pit) {
- pos_type pos = pars_[pit].size();
-
- if (pars_[pit].isInserted(pos)) {
- pars_[pit].setChange(pos, Change(Change::UNCHANGED));
- } else if (pars_[pit].isDeleted(pos)) {
- if (pit == pars_size - 1) {
-// we cannot remove a par break at the end of the last
-// paragraph; instead, we mark it unchanged
-pars_[pit].setChange(pos, Change(Change::UNCHANGED));
- } else {
-mergeParagraph(bparams, pars_, pit);
---pit;
---pars_size;
- }
- }
- }
-
+ lyx::acceptChanges(pars_, bparams);
// finally, invoke the DEPM
- deleteEmptyParagraphMechanism(0, pars_size - 1, bparams.trackChanges);
+ deleteEmptyParagraphMechanism(0, pars_.size() - 1, bparams.trackChanges);
}
Index: CutAndPaste.cpp
===
--- CutAndPaste.cpp (revision 18885)
+++ CutAndPaste.cpp (working copy)
@@ -388,12 +388,7 @@
}
// do not copy text (also nested in insets) which is marked as deleted
- // acceptChanges() is defined for Text rather than ParagraphList
- // Thus we must wrap copy_pars into a Text object and cross our fingers
- Text lt;
- copy_pars.swap(lt.paragraphs());
- lt.acceptChanges(buf.params());
- copy_pars.swap(lt.paragraphs());
+ acceptChanges(copy_pars, buf.params());
cutstack.push(make_pair(copy_pars, tc));
}
@@ -723,7 +718,7 @@
textclass, errorList);
updateLabels(cur.buffer());
cur.clearSelection();
- text->setCursor(cur.top(), ppp.first, ppp.second);
+ text->setCursor(cur, ppp.first, ppp.second);
}
// mathed is handled in InsetMathNest/InsetMathGrid
Index: paragraph_funcs.h
===
--- paragraph_funcs.h (revision 18885)
+++ paragraph_funcs.h (working copy)
@@ -75,7 +75,10 @@
/// return the number of InsetOptArg in a paragraph
int numberOfOptArgs(Paragraph const & par);
+/// accept the changes within the complete ParagraphList
+void acceptChanges(ParagraphList & pars, BufferParams const & bparams);
+
} // namespace lyx
#endif // PARAGRAPH_FUNCS_H
Index: paragraph_funcs.cpp
===
--- paragraph_funcs.cpp (revision 18885)
+++ paragraph_funcs.cpp (working copy)
@@ -317,4 +317,36 @@
}
+void acceptChanges(ParagraphList & pars, BufferParams const & bparams)
+{
+ pit_type pars_size = static_cast(pars.size());
+
+ // first, accept changes within each individual paragraph
+ // (do not consider end-of-par)
+ for (pit_type pit = 0; pit < pars_size; ++pit) {
+ if (!pars[pit].empty()) // prevent assertion failure
+ pars[pit].acceptChanges(bparams, 0, pars[pit].size());
+ }
+
+ // next, accept imaginary end-of-par characters
+ for (pit_type pit = 0; pit < pars_size; ++pit) {
+ pos_type pos = pars[pit].size();
+
+ if (pars[pit].isInserted(pos)) {
+ pars[pit].setChange(pos, Change(Change::UNCHANGED));
+ } else if (pars[pit].isDeleted(pos)) {
+ if (pit == pars_size - 1) {
+// we cannot remove a par break at the end of the last
+// paragraph; instead, we mark it unchanged
+pars[pit].setChange(pos, Change(Change::UNCHANGED));
+ } else {
+mergeParagraph(bparams, pars, pit);
+--pit;
+--pars_size;
+ }
+ }
+ }
+}
+
+
} // namespace lyx
Re: [patch] fix bug 3886: Cut/paste bug with environment depths in 1.5svn
> "Alfredo" == Alfredo Braunstein <[EMAIL PROTECTED]> writes: Alfredo> Seems DEPM was not called after insertion, the following Alfredo> change to pasteParagraphList (included in the patch) seems to Alfredo> fix it: Alfredo> - text->setCursor(cur.top(), ppp.first, ppp.second); + Alfredo> text->setCursor(cur, ppp.first, ppp.second); Indeed. setCursor(CursorSlice,...) does not call depm. These two methods sharing the same name are very misleading. JMarc
Re: [patch] fix bug 3886: Cut/paste bug with environment depths in 1.5svn
On 6/26/07, Jean-Marc Lasgouttes <[EMAIL PROTECTED]> wrote: So now we have patch in one place and the explanation in another place... http://bugzilla.lyx.org/attachment.cgi?id=1927&action=view ;-) If only the list wouldn't take 1 hour to get my message in... A/
Re: [patch] fix bug 3886: Cut/paste bug with environment depths in 1.5svn
Alfredo Braunstein wrote: Alfredo Braunstein wrote: The problem has nothing to do with env depths, just with cut/past of empty paragraphs. The problem is that Text::acceptChanges on the internal cut/paste paragraph list calls DEPM and this removes an empty paragraph at the beginning. The fix is to move the bulk of acceptChanges to paragraph_funcs (where it belongs IMO) and the version in Text just calls that and then DEPM, while in cap we just call the plain version. ... and the patch. I think this is the right thing to do, but on a first test this uncovers a DEPM bug :-O ... Seems DEPM was not called after insertion, the following change to pasteParagraphList (included in the patch) seems to fix it: - text->setCursor(cur.top(), ppp.first, ppp.second); + text->setCursor(cur, ppp.first, ppp.second); Looks good Alfredo but one question: why don't you just move deleteEmptyParagraphMechanism() to paragraph_funcs as well? Same thing for Text::fixCursorAfterDelete(), this has nothing to do with Text AFAIS... Abdel.
Re: [patch] fix bug 3886: Cut/paste bug with environment depths in 1.5svn
> "Alfredo" == Alfredo Braunstein <[EMAIL PROTECTED]> writes: Alfredo> Alfredo Braunstein wrote: >> The problem has nothing to do with env depths, just with cut/past >> of empty paragraphs. The problem is that Text::acceptChanges on the >> internal cut/paste paragraph list calls DEPM and this removes an >> empty paragraph at the beginning. >> >> The fix is to move the bulk of acceptChanges to paragraph_funcs >> (where it belongs IMO) and the version in Text just calls that and >> then DEPM, while in cap we just call the plain version. Alfredo> ... and the patch. I like it (fixes a bug, does useful cleanup). You should probably remove the // finally, invoke the DEPM comment which looks a bit strange in such a small method. OK JMarc
Re: [patch] fix bug 3886: Cut/paste bug with environment depths in 1.5svn
Abdelrazak Younes wrote: > Looks good Alfredo but one question: why don't you just move > deleteEmptyParagraphMechanism() to paragraph_funcs as well? Same thing > for Text::fixCursorAfterDelete(), this has nothing to do with Text > AFAIS... I see your point. However, I think they don't belong to paragraph_funcs because both involve Cursors while the other functions in paragraph_funcs are more elementary. In any case this is orthogonal to the patch, as I don't call DEPM directly (neither before nor after the patch). I propose to leave this reorganization for after 1.5.0. A/
Re: [patch] fix bug 3886: Cut/paste bug with environment depths in 1.5svn
Alfredo Braunstein wrote: Abdelrazak Younes wrote: Looks good Alfredo but one question: why don't you just move deleteEmptyParagraphMechanism() to paragraph_funcs as well? Same thing for Text::fixCursorAfterDelete(), this has nothing to do with Text AFAIS... I see your point. However, I think they don't belong to paragraph_funcs because both involve Cursors while the other functions in paragraph_funcs are more elementary. Hum... right. Except that we should not need a Cursor but a DocIterator in an ideal world... In such ideal word, the methods should then go to buffer_funcs. In any case this is orthogonal to the patch, as I don't call DEPM directly (neither before nor after the patch). I propose to leave this reorganization for after 1.5.0. OK. IMHO such cleanup should go in 1.5.x but not everybody agrees with me... Abdel.
Re: [patch] fix bug 3886: Cut/paste bug with environment depths in 1.5svn
> "Alfredo" == Alfredo Braunstein <[EMAIL PROTECTED]> writes: Alfredo> Abdelrazak Younes wrote: >> Looks good Alfredo but one question: why don't you just move >> deleteEmptyParagraphMechanism() to paragraph_funcs as well? Same >> thing for Text::fixCursorAfterDelete(), this has nothing to do with >> Text AFAIS... Alfredo> I see your point. However, I think they don't belong to Alfredo> paragraph_funcs because both involve Cursors while the other Alfredo> functions in paragraph_funcs are more elementary. I also prefer it like you did. JMarc
Re: [patch] fix bug 3886: Cut/paste bug with environment depths in 1.5svn
Jean-Marc Lasgouttes wrote: > I like it (fixes a bug, does useful cleanup). You should probably remove > the // finally, invoke the DEPM > comment which looks a bit strange in such a small method. Done. > OK I commited it. A/
Re: [patch] fix bug 3886: Cut/paste bug with environment depths in 1.5svn
Alfredo Braunstein wrote: Jean-Marc Lasgouttes wrote: I like it (fixes a bug, does useful cleanup). You should probably remove the // finally, invoke the DEPM comment which looks a bit strange in such a small method. Done. OK I commited it. Tested, and "paste" now works as expected. Thanks for the effort! Cut is still not as expected - an empty paragraph is left over. Of course it disappears when the cursor is moved, like any other empty paragraph. So, not a real problem, but it surprised me a little. Helge Hafting
