Make sure that undo is recorded when magic tricks are played with InsetBibitem.

Fixes bug #7111: Assertion with undo and InsetBibitem

  * add Paragraph::brokenBiblio(), with tells whether there is something to fix.
  * rename Paragraph::checkBiblio to fixBiblio; simplify it greatly by using
    InsetList methods
  * In TextMetrics::redoParagraph, call recordUndo before fixBiblio (only when
    there is something to fix obviously)

Special care is taken to update cursor only when it makes sense. Note
that there are cases where undo information is not recorded (current
cursor not in the slice where action happens)

diff --git a/src/Paragraph.cpp b/src/Paragraph.cpp
index 29092b03be835ca674636b13d4f4a063d745ffa1..389e29267c191653718376d5c523648b9a3c1feb 100644 (file)
--- a/src/Paragraph.cpp
+++ b/src/Paragraph.cpp
@@ -3167,70 +3167,61 @@ char_type Paragraph::transformChar(char_type c, pos_type pos) const
 }
 
 
-int Paragraph::checkBiblio(Buffer const & buffer)
+bool Paragraph::brokenBiblio() const
 {
-       // FIXME From JS:
-       // This is getting more and more a mess. ...We really should clean
-       // up this bibitem issue for 1.6.
+       // there is a problem if there is no bibitem at position 0 or
+       // if there is another bibitem in the paragraph.
+       return d->layout_->labeltype == LABEL_BIBLIO
+               && (d->insetlist_.find(BIBITEM_CODE) != 0
+                   || d->insetlist_.find(BIBITEM_CODE, 1) > 0);
+}
+
+
+int Paragraph::fixBiblio(Buffer const & buffer)
+{
+       // FIXME: What about the case where paragraph is not BIBLIO
+       // but there is an InsetBibitem?
+       // FIXME: when there was already an inset at 0, the return value is 1,
+       // which does not tell whether another inset has been remove; the
+       // cursor cannot be correctly updated.
 
-       // Add bibitem insets if necessary
        if (d->layout_->labeltype != LABEL_BIBLIO)
                return 0;
 
-       bool hasbibitem = !d->insetlist_.empty()
-               // Insist on it being in pos 0
-               && d->text_[0] == META_INSET
-               && d->insetlist_.begin()->inset->lyxCode() == BIBITEM_CODE;
-
-       bool track_changes = buffer.params().trackChanges;
-
-       docstring oldkey;
-       docstring oldlabel;
-
-       // remove a bibitem in pos != 0
-       // restore it later in pos 0 if necessary
-       // (e.g. if a user inserts contents _before_ the item)
-       // we're assuming there's only one of these, which there
-       // should be.
-       int erasedInsetPosition = -1;
-       InsetList::iterator it = d->insetlist_.begin();
-       InsetList::iterator end = d->insetlist_.end();
-       for (; it != end; ++it)
-               if (it->inset->lyxCode() == BIBITEM_CODE
-                     && it->pos > 0) {
-                       InsetCommand * olditem = it->inset->asInsetCommand();
-                       oldkey = olditem->getParam("key");
-                       oldlabel = olditem->getParam("label");
-                       erasedInsetPosition = it->pos;
-                       eraseChar(erasedInsetPosition, track_changes);
-                       break;
-       }
+       bool const track_changes = buffer.params().trackChanges;
+       int bibitem_pos = d->insetlist_.find(BIBITEM_CODE);
+       bool const hasbibitem0 = bibitem_pos == 0;
 
-       // There was an InsetBibitem at the beginning, and we didn't
-       // have to erase one.
-       if (hasbibitem && erasedInsetPosition < 0)
+       if (hasbibitem0) {
+               bibitem_pos = d->insetlist_.find(BIBITEM_CODE, 1);
+               // There was an InsetBibitem at pos 0, and no other one => OK
+               if (bibitem_pos == -1)
                        return 0;
+               // there is a bibitem at the 0 position, but since
+               // there is a second one, we copy the second on the
+               // first. We're assuming there are at most two of
+               // these, which there should be.
+               // FIXME: why does it make sense to do that rather
+               // than keep the first? (JMarc)
+               Inset * inset = d->insetlist_.release(bibitem_pos);
+               eraseChar(bibitem_pos, track_changes);
+               d->insetlist_.begin()->inset = inset;
+               return -bibitem_pos;
+       }
+
+       // We need to create an inset at the beginning
+       Inset * inset = 0;
+       if (bibitem_pos > 0) {
+               // there was one somewhere in the paragraph, let's move it
+               inset = d->insetlist_.release(bibitem_pos);
+               eraseChar(bibitem_pos, track_changes);
+       } else
+               // make a fresh one
+               inset = new InsetBibitem(const_cast<Buffer *>(&buffer),
+                                        InsetCommandParams(BIBITEM_CODE));
 
-       // There was an InsetBibitem at the beginning and we did have to
-       // erase one. So we give its properties to the beginning inset.
-       if (hasbibitem) {
-               InsetCommand * inset = d->insetlist_.begin()->inset->asInsetCommand();
-               if (!oldkey.empty())
-                       inset->setParam("key", oldkey);
-               inset->setParam("label", oldlabel);
-               return -erasedInsetPosition;
-       }
-
-       // There was no inset at the beginning, so we need to create one with
-       // the key and label of the one we erased.
-       InsetBibitem * inset =
-               new InsetBibitem(const_cast<Buffer *>(&buffer), InsetCommandParams(BIBITEM_CODE));
-       // restore values of previously deleted item in this par.
-       if (!oldkey.empty())
-               inset->setParam("key", oldkey);
-       inset->setParam("label", oldlabel);
-       insertInset(0, inset,
-                   Change(track_changes ? Change::INSERTED : Change::UNCHANGED));
+       insertInset(0, inset, Change(track_changes ? Change::INSERTED 
+                                                  : Change::UNCHANGED));
 
        return 1;
 }

diff --git a/src/Paragraph.h b/src/Paragraph.h
index f5d6a23b26f9b18f20f47542ab3a6c2cdf870c62..0ea959fd544804779b3589392da91b8bb86109ea 100644 (file)
--- a/src/Paragraph.h
+++ b/src/Paragraph.h
@@ -440,6 +440,8 @@ public:
        ///
        ParagraphParameters const & params() const;
 
+       /// Check whether a call to fixBiblio is needed.
+       bool brokenBiblio() const;
        /// Check if we are in a Biblio environment and insert or
        /// delete InsetBibitems as necessary.
        /// \retval int 1, if we had to add an inset, in which case
@@ -447,7 +449,7 @@ public:
        /// an inset, in which case pos is the position from which the inset
        /// was deleted, and the cursor will need to be moved back one if it
        /// was previously past that position. Return 0 otherwise.
-       int checkBiblio(Buffer const & buffer);
+       int fixBiblio(Buffer const & buffer);
 
        /// For each author, set 'used' to true if there is a change
        /// by this author in the paragraph.

diff --git a/src/TextMetrics.cpp b/src/TextMetrics.cpp
index 10e89046569005f815d10de212a6d7baeb875d11..8831ae938450d2685304d4184d488bc37fcbc859 100644 (file)
--- a/src/TextMetrics.cpp
+++ b/src/TextMetrics.cpp
@@ -375,19 +375,28 @@ bool TextMetrics::redoParagraph(pit_type const pit)
        main_text_ = (text_ == &buffer.text());
        bool changed = false;
 
+       // Check whether there are InsetBibItems that need fixing
        // FIXME: This check ought to be done somewhere else. It is the reason
-       // why text_ is not     const. But then, where else to do it?
+       // why text_ is not const. But then, where else to do it?
        // Well, how can you end up with either (a) a biblio environment that
        // has no InsetBibitem or (b) a biblio environment with more than one
        // InsetBibitem? I think the answer is: when paragraphs are merged;
        // when layout is set; when material is pasted.
-       int const moveCursor = par.checkBiblio(buffer);
-       if (moveCursor > 0)
-               const_cast<Cursor &>(bv_->cursor()).posForward();
-       else if (moveCursor < 0) {
-               Cursor & cursor = const_cast<Cursor &>(bv_->cursor());
-               if (cursor.pos() >= -moveCursor)
-                       cursor.posBackward();
+       if (par.brokenBiblio()) {
+               Cursor & cur = const_cast<Cursor &>(bv_->cursor());
+               // In some cases, we do not know how to record undo
+               if (&cur.inset() == &text_->inset())
+                       cur.recordUndo(ATOMIC_UNDO, pit, pit);
+
+               int const moveCursor = par.fixBiblio(buffer);
+
+               // Is it necessary to update the cursor?
+               if (&cur.inset() == &text_->inset() && cur.pit() == pit) {
+                       if (moveCursor > 0)
+                               cur.posForward();
+                       else if (moveCursor < 0 && cur.pos() >= -moveCursor)
+                               cur.posBackward();
+               }
        }
 
        // Optimisation: this is used in the next two loops

diff --git a/status.20x b/status.20x
index 1611822935839a1d02ca941e88d225a2fa422c10..e99432554a6513b7316c9d27f3407f83ddea53a9 100644 (file)
--- a/status.20x
+++ b/status.20x
@@ -95,6 +95,8 @@ What's new
 - Fix various crashes when single document is edited in more windows
   (bug 8203).
 
+- Fix crash when using undo in a paragraph with layout Bibliography (bug 7111).
+
 - Replace current selection when pasting (bug 8027).
 
 - Make sure that undo restores environment depth correctly (bug 8159).