From c999e35e54eca6e0b736a034d415b6a90a866bc0 Mon Sep 17 00:00:00 2001 From: Jean-Marc Lasgouttes Date: Tue, 17 Jul 2012 23:59:04 +0200 Subject: [PATCH] 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 | 101 ++++++++++++++++++++------------------------ src/Paragraph.h | 4 +- src/TextMetrics.cpp | 25 +++++++---- 3 files changed, 66 insertions(+), 64 deletions(-) diff --git a/src/Paragraph.cpp b/src/Paragraph.cpp index 81d3acd1be..d93594a678 100644 --- a/src/Paragraph.cpp +++ b/src/Paragraph.cpp @@ -3229,70 +3229,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), 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), + InsetCommandParams(BIBITEM_CODE)); + + insertInset(0, inset, Change(track_changes ? Change::INSERTED + : Change::UNCHANGED)); return 1; } diff --git a/src/Paragraph.h b/src/Paragraph.h index f5d6a23b26..0ea959fd54 100644 --- 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 f291e397f0..9382230509 100644 --- 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(bv_->cursor()).posForward(); - else if (moveCursor < 0) { - Cursor & cursor = const_cast(bv_->cursor()); - if (cursor.pos() >= -moveCursor) - cursor.posBackward(); + if (par.brokenBiblio()) { + Cursor & cur = const_cast(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