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)

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 const track_changes = buffer.params().trackChanges;
+	int bibitem_pos = d->insetlist_.find(BIBITEM_CODE);
+	bool const hasbibitem0 = bibitem_pos == 0;
 
-	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;
-	}
-
-	// 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 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 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;
 	}
 
-	// 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));
+	// 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));
+
+	insertInset(0, inset, Change(track_changes ? Change::INSERTED 
+				                   : Change::UNCHANGED));
 
 	return 1;
 }

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.

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

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).