Fix bug #5204: Undo removes text selection

This commit does a bit more than fix selection, since it saves the full cursor state
in the undo stack. This means that undo now restores:
* the selection
* the current font
* transient mark (shall we keep this one?), logical position...

In order to do that, it introduces an intermediate class between Cursor and DotIterator: CursorData.
The new inheritance diagram is thus
  DocIteraator -> CursorData -> Cursor

CursorData contains all the members of Cursor that define the current position, but not the stuff
related to current view of dispatch mechanism. It may make sense in the future to move members
between CursorData and Cursor and to move some member functions to CursorData.

Now UndoElement uses CursorData for cur_before and cur_after, but not for the cell. The undo API uses
also CursorData instead of DocIterator.

src/Buffer.cpp

@@ -24,6 +24,7 @@
 #include "Chktex.h"
 #include "Converter.h"
 #include "Counters.h"
+#include "Cursor.h"
 #include "DispatchResult.h"
 #include "DocIterator.h"
 #include "Encoding.h"
@@ -2353,7 +2354,7 @@ void Buffer::dispatch(FuncRequest const & func, DispatchResult & dr)
 			if (it->lyxCode() == BRANCH_CODE) {
 				InsetBranch & ins = static_cast<InsetBranch &>(*it);
 				if (ins.branch() == oldname) {
-					undo().recordUndo(it);
+					undo().recordUndo(CursorData(it));
 					ins.rename(newname);
 					success = true;
 					continue;
@@ -4442,7 +4443,7 @@ void Buffer::updateBuffer(ParIterator & parit, UpdateType utype) const
 			 * non-const. This would however be costly in
 			 * terms of code duplication.
 			 */
-			const_cast<Buffer *>(this)->undo().recordUndo(parit);
+			const_cast<Buffer *>(this)->undo().recordUndo(CursorData(parit));
 			parit->params().depth(maxdepth);
 		}
 		maxdepth = parit->getMaxDepthAfter();

src/Cursor.cpp

@@ -250,13 +250,32 @@ bool bruteFind3(Cursor & cur, int x, int y, bool up)
 } // namespace anon
 
 
+CursorData::CursorData()
+	: DocIterator(), anchor_(),
+	  selection_(false), mark_(false), word_selection_(false),
+	  logicalpos_(false), current_font(inherit_font)
+{}
+
+
+CursorData::CursorData(Buffer * buffer)
+	: DocIterator(buffer), anchor_(),
+	  selection_(false), mark_(false), word_selection_(false),
+	  logicalpos_(false), current_font(inherit_font)
+{}
+
+
+CursorData::CursorData(DocIterator const & dit)
+	: DocIterator(dit), anchor_(),
+	  selection_(false), mark_(false), word_selection_(false),
+	  logicalpos_(false), current_font(inherit_font)
+{}
+
+
 // be careful: this is called from the bv's constructor, too, so
 // bv functions are not yet available!
 Cursor::Cursor(BufferView & bv)
-	: DocIterator(&bv.buffer()), bv_(&bv), anchor_(),
+	: CursorData(&bv.buffer()), bv_(&bv), 
-	  x_target_(-1), textTargetOffset_(0),
+	  x_target_(-1), textTargetOffset_(0)
-	  selection_(false), mark_(false), word_selection_(false),
-	  logicalpos_(false), current_font(inherit_font)
 {}
 
 
@@ -279,6 +298,25 @@ void Cursor::setCursor(DocIterator const & cur)
 }
 
 
+void Cursor::setCursorToAnchor()
+{
+	if (selection()) {
+		DocIterator normal = anchor_;
+		while (depth() < normal.depth())
+			normal.pop_back();
+		if (depth() < anchor_.depth() && top() <= anchor_[depth() - 1])
+			++normal.pos();
+		setCursor(normal);
+	}
+}
+
+
+void Cursor::setCursorData(CursorData const & data)
+{
+	CursorData::operator=(data);
+}
+
+
 bool Cursor::getStatus(FuncRequest const & cmd, FuncStatus & status) const
 {
 	Cursor cur = *this;
@@ -505,19 +543,6 @@ void Cursor::resetAnchor()
 }
 
 
-void Cursor::setCursorToAnchor()
-{
-	if (selection()) {
-		DocIterator normal = anchor_;
-		while (depth() < normal.depth())
-			normal.pop_back();
-		if (depth() < anchor_.depth() && top() <= anchor_[depth() - 1])
-			++normal.pos();
-		setCursor(normal);
-	}
-}
-
-
 void Cursor::markNewWordPosition()
 {
 	if (lyxrc.spellcheck_continuously && inTexted() && new_word_.empty()) {
@@ -2373,13 +2398,8 @@ void Cursor::setCurrentFont()
 
 bool Cursor::textUndo()
 {
-	DocIterator dit = *this;
+	if (!buffer()->undo().textUndo(*this))
-	// Undo::textUndo() will modify dit.
-	if (!buffer()->undo().textUndo(dit))
 		return false;
-	// Set cursor
-	setCursor(dit);
-	clearSelection();
 	fixIfBroken();
 	return true;
 }
@@ -2387,13 +2407,8 @@ bool Cursor::textUndo()
 
 bool Cursor::textRedo()
 {
-	DocIterator dit = *this;
+	if (!buffer()->undo().textRedo(*this))
-	// Undo::textRedo() will modify dit.
-	if (!buffer()->undo().textRedo(dit))
 		return false;
-	// Set cursor
-	setCursor(dit);
-	clearSelection();
 	fixIfBroken();
 	return true;
 }

src/Cursor.h

@@ -34,9 +34,61 @@ class Row;
 class InsetMathUnknown;
 class Encoding;
 
+/**
+ * This class describes the position of a cursor within a document,
+ * but does not contain any detail about the view. It is currently
+ * only used to save cursor position in Undo, but culd be extended to
+ * handle the methods that only need this data.
+ **/
+class CursorData : public DocIterator
+{
+public:
+	///
+	CursorData();
+	///
+	explicit CursorData(Buffer * buffer);
+	///
+	explicit CursorData(DocIterator const & dit);
+protected:
+	/// the anchor position
+	DocIterator anchor_;
+	/// the start of the new born word
+	DocIterator new_word_;
+	///
+	mutable DispatchResult disp_;
+	/// do we have a selection?
+	bool selection_;
+	/// are we on the way to get one?
+	bool mark_;
+	/// are we in word-selection mode? This is set when double clicking.
+	bool word_selection_;
+	/// If true, we are behind the previous char, otherwise we are in front
+	// of the next char. This only make a difference when we are in front
+	// of a big inset spanning a whole row and computing coordinates for
+	// displaying the cursor.
+	bool logicalpos_;
+
+// FIXME: make them protected.
+public:
+	/// the current font settings
+	Font current_font;
+	/// the current font
+	Font real_current_font;
+
+protected:
+
+	//
+	// math specific stuff that could be promoted to "global" later
+	//
+	/// do we allow autocorrection
+	bool autocorrect_;
+	/// are we entering a macro name?
+	bool macromode_;
+};
+
 
 /// The cursor class describes the position of a cursor within a document.
-class Cursor : public DocIterator
+class Cursor : public CursorData
 {
 public:
 	/// create the cursor of a BufferView
@@ -60,6 +112,8 @@ public:
 	bool popForward();
 	/// make sure we are outside of given inset
 	void leaveInset(Inset const & inset);
+	/// set the cursor data
+	void setCursorData(CursorData const & data);
 	/// sets cursor part
 	void setCursor(DocIterator const & it);
 	/// sets the cursor to the normalized selection anchor
@@ -320,10 +374,6 @@ private:
 private:
 	///
 	BufferView * bv_;
-	/// the anchor position
-	DocIterator anchor_;
-	/// the start of the new born word
-	DocIterator new_word_;
 	///
 	mutable DispatchResult disp_;
 	/**
@@ -341,42 +391,12 @@ private:
 	int x_target_;
 	/// if a x_target cannot be hit exactly in a text, put the difference here
 	int textTargetOffset_;
-	/// do we have a selection?
-	bool selection_;
-	/// are we on the way to get one?
-	bool mark_;
-	/// are we in word-selection mode? This is set when double clicking.
-	bool word_selection_;
-	/// If true, we are behind the previous char, otherwise we are in front
-	// of the next char. This only make a difference when we are in front
-	// of a big inset spanning a whole row and computing coordinates for
-	// displaying the cursor.
-	bool logicalpos_;
 	/// position before dispatch started
 	DocIterator beforeDispatchCursor_;
 	/// cursor screen coordinates before dispatch started
 	int beforeDispatchPosX_;
 	int beforeDispatchPosY_;
 
-
-// FIXME: make them private.
-public:
-	/// the current font settings
-	Font current_font;
-	/// the current font
-	Font real_current_font;
-
-private:
-
-	//
-	// math specific stuff that could be promoted to "global" later
-	//
-	/// do we allow autocorrection
-	bool autocorrect_;
-	/// are we entering a macro name?
-	bool macromode_;
-
-
 ///////////////////////////////////////////////////////////////////
 //
 // The part below is the non-integrated rest of the original math

src/Undo.cpp

@@ -20,7 +20,7 @@
 #include "Buffer.h"
 #include "BufferParams.h"
 #include "buffer_funcs.h"
-#include "DocIterator.h"
+#include "Cursor.h"
 #include "Paragraph.h"
 #include "ParagraphList.h"
 #include "Text.h"
@@ -65,7 +65,7 @@ where to insert the stored bits when performining undo.
 struct UndoElement
 {
 	///
-	UndoElement(UndoKind kin, StableDocIterator const & cb, 
+	UndoElement(UndoKind kin, CursorData const & cb, 
 	            StableDocIterator const & cel,
 	            pit_type fro, pit_type en, ParagraphList * pl, 
 	            MathData * ar, BufferParams const & bp, 
@@ -103,9 +103,9 @@ struct UndoElement
 	/// Which kind of operation are we recording for?
 	UndoKind kind;
 	/// the position of the cursor before recordUndo
-	StableDocIterator cur_before;
+	CursorData cur_before;
 	/// the position of the cursor at the end of the undo group
-	StableDocIterator cur_after;
+	CursorData cur_after;
 	/// the position of the cell described
 	StableDocIterator cell;
 	/// counted from begin of cell
@@ -188,17 +188,17 @@ struct Undo::Private
 				   group_id(0), group_level(0) {}
 	
 	// Do one undo/redo step
-	void doTextUndoOrRedo(DocIterator & cur, UndoElementStack & stack, 
+	void doTextUndoOrRedo(CursorData & cur, UndoElementStack & stack, 
 			      UndoElementStack & otherStack);
 	// Apply one undo/redo group. Returns false if no undo possible.
-	bool textUndoOrRedo(DocIterator & cur, bool isUndoOperation);
+	bool textUndoOrRedo(CursorData & cur, bool isUndoOperation);
 
 	///
 	void doRecordUndo(UndoKind kind,
 		DocIterator const & cell,
 		pit_type first_pit,
 		pit_type last_pit,
-		StableDocIterator const & cur,
+		CursorData const & cur,
 		bool isFullBuffer,
 		UndoElementStack & stack);
 	///
@@ -206,7 +206,7 @@ struct Undo::Private
 		DocIterator const & cell,
 		pit_type first_pit,
 		pit_type last_pit,
-		DocIterator const & cur,
+		CursorData const & cur,
 		bool isFullBuffer);
 
 	///
@@ -291,7 +291,7 @@ static bool samePar(StableDocIterator const & i1, StableDocIterator const & i2)
 void Undo::Private::doRecordUndo(UndoKind kind,
 	DocIterator const & cell,
 	pit_type first_pit, pit_type last_pit,
-	StableDocIterator const & cur_before,
+	CursorData const & cur_before,
 	bool isFullBuffer,
 	UndoElementStack & stack)
 {
@@ -316,7 +316,7 @@ void Undo::Private::doRecordUndo(UndoKind kind,
 	    && stack.top().from == from
 	    && stack.top().end == end) {
 		// reset cur_after; it will be filled correctly by endUndoGroup.
-		stack.top().cur_after = StableDocIterator();
+		stack.top().cur_after = CursorData();
 		return;
 	}
 
@@ -356,7 +356,7 @@ void Undo::Private::doRecordUndo(UndoKind kind,
 void Undo::Private::recordUndo(UndoKind kind,
 			       DocIterator const & cell,
 			       pit_type first_pit, pit_type last_pit,
-			       DocIterator const & cur,
+			       CursorData const & cur,
 			       bool isFullBuffer)
 {
 	LASSERT(first_pit <= cell.lastpit(), /**/);
@@ -378,7 +378,7 @@ void Undo::Private::recordUndo(UndoKind kind,
 }
 
 
-void Undo::Private::doTextUndoOrRedo(DocIterator & cur, UndoElementStack & stack, UndoElementStack & otherstack)
+void Undo::Private::doTextUndoOrRedo(CursorData & cur, UndoElementStack & stack, UndoElementStack & otherstack)
 {
 	// Adjust undo stack and get hold of current undo data.
 	UndoElement & undo = stack.top();
@@ -446,8 +446,8 @@ void Undo::Private::doTextUndoOrRedo(DocIterator & cur, UndoElementStack & stack
 	LASSERT(undo.pars == 0, /**/);
 	LASSERT(undo.array == 0, /**/);
 
-	if (undo.cur_before.size())
+	if (!undo.cur_before.empty())
-		cur = undo.cur_before.asDocIterator(&buffer_);
+		cur = undo.cur_before;
 	if (undo.lyx_clean)
 		buffer_.markClean();
 	else
@@ -457,7 +457,7 @@ void Undo::Private::doTextUndoOrRedo(DocIterator & cur, UndoElementStack & stack
 }
 
 
-bool Undo::Private::textUndoOrRedo(DocIterator & cur, bool isUndoOperation)
+bool Undo::Private::textUndoOrRedo(CursorData & cur, bool isUndoOperation)
 {
 	undo_finished_ = true;
 
@@ -486,13 +486,13 @@ void Undo::finishUndo()
 }
 
 
-bool Undo::textUndo(DocIterator & cur)
+bool Undo::textUndo(CursorData & cur)
 {
 	return d->textUndoOrRedo(cur, true);
 }
 
 
-bool Undo::textRedo(DocIterator & cur)
+bool Undo::textRedo(CursorData & cur)
 {
 	return d->textUndoOrRedo(cur, false);
 }
@@ -521,10 +521,10 @@ void Undo::endUndoGroup()
 }
 
 
-void Undo::endUndoGroup(DocIterator const & cur)
+void Undo::endUndoGroup(CursorData const & cur)
 {
 	endUndoGroup();
-	if (!d->undostack_.empty() && !d->undostack_.top().cur_after.size())
+	if (!d->undostack_.empty() && d->undostack_.top().cur_after.empty())
 		d->undostack_.top().cur_after = cur;
 }
 
@@ -533,13 +533,13 @@ void Undo::endUndoGroup(DocIterator const & cur)
 // Private::recordUndo public as sole interface. The code in the
 // convenience functions can move to Cursor.cpp.
 
-void Undo::recordUndo(DocIterator const & cur, UndoKind kind)
+void Undo::recordUndo(CursorData const & cur, UndoKind kind)
 {
 	d->recordUndo(kind, cur, cur.pit(), cur.pit(), cur, false);
 }
 
 
-void Undo::recordUndoInset(DocIterator const & cur, UndoKind kind,
+void Undo::recordUndoInset(CursorData const & cur, UndoKind kind,
 			   Inset const * inset)
 {
 	if (!inset || inset == &cur.inset()) {
@@ -553,20 +553,20 @@ void Undo::recordUndoInset(DocIterator const & cur, UndoKind kind,
 }
 
 
-void Undo::recordUndo(DocIterator const & cur, UndoKind kind, pit_type from)
+void Undo::recordUndo(CursorData const & cur, UndoKind kind, pit_type from)
 {
 	d->recordUndo(kind, cur, cur.pit(), from, cur, false);
 }
 
 
-void Undo::recordUndo(DocIterator const & cur, UndoKind kind,
+void Undo::recordUndo(CursorData const & cur, UndoKind kind,
 	pit_type from, pit_type to)
 {
 	d->recordUndo(kind, cur, from, to, cur, false);
 }
 
 
-void Undo::recordUndoFullDocument(DocIterator const & cur)
+void Undo::recordUndoFullDocument(CursorData const & cur)
 {
 	// This one may happen outside of the main undo group, so we
 	// put it in its own subgroup to avoid complaints.

src/Undo.h

@@ -23,7 +23,7 @@ namespace lyx {
 
 class Buffer;
 class BufferParams;
-class DocIterator;
+class CursorData;
 class Inset;
 class MathData;
 class ParagraphList;
@@ -57,10 +57,10 @@ public:
 	void clear();
 
 	/// this will undo the last action - returns false if no undo possible
-	bool textUndo(DocIterator &);
+	bool textUndo(CursorData &);
 
 	/// this will redo the last undo - returns false if no redo possible
-	bool textRedo(DocIterator &);
+	bool textRedo(CursorData &);
 
 	/// End a sequence of INSERT_UNDO or DELETE_UNDO type of undo
 	/// operations (grouping of consecutive characters insertion/deletion).
@@ -85,7 +85,7 @@ public:
 	void endUndoGroup();
 
 	/// end the current undo group and set UndoElement::cur_after if necessary.
-	void endUndoGroup(DocIterator const &);
+	void endUndoGroup(CursorData const &);
 
 	/// The general case: record undo information for an arbitrary range.
 	/**
@@ -95,25 +95,25 @@ public:
 	 * changes to the paragraph, and it will record the original
 	 * information of the paragraphs in the undo stack.
 	 */
-	void recordUndo(DocIterator const & cur, UndoKind kind,
+	void recordUndo(CursorData const & cur, UndoKind kind,
 		pit_type from, pit_type to);
 
 	/// Convenience: record undo information for the range between
 	/// 'from' and cursor.
-	void recordUndo(DocIterator const & cur, UndoKind kind, pit_type from);
+	void recordUndo(CursorData const & cur, UndoKind kind, pit_type from);
 
 	/// Convenience: record undo information for the single
 	/// paragraph or cell containing the cursor.
-	void recordUndo(DocIterator const & cur, UndoKind kind = ATOMIC_UNDO);
+	void recordUndo(CursorData const & cur, UndoKind kind = ATOMIC_UNDO);
 
 	/// Convenience: record undo information for the inset
 	/// containing the cursor.
-	void recordUndoInset(DocIterator const & cur,
+	void recordUndoInset(CursorData const & cur,
 			     UndoKind kind = ATOMIC_UNDO,
 			     Inset const * inset = 0);
 
 	/// Convenience: prepare undo for the whole buffer
-	void recordUndoFullDocument(DocIterator const & cur);
+	void recordUndoFullDocument(CursorData const & cur);
 
 private:
 	struct Private;

src/insets/InsetGraphics.cpp

@@ -1164,7 +1164,7 @@ void unifyGraphicsGroups(Buffer & b, string const & argument)
 			InsetGraphics & ins = static_cast<InsetGraphics &>(*it);
 			InsetGraphicsParams inspar = ins.getParams();
 			if (params.groupId == inspar.groupId) {
-				b.undo().recordUndo(it);
+				b.undo().recordUndo(CursorData(it));
 				params.filename = inspar.filename;
 				ins.setParams(params);
 			}

src/insets/InsetLabel.cpp

@@ -111,7 +111,7 @@ void InsetLabel::updateReferences(docstring const & old_label,
 	Buffer::References::iterator it = refs.begin();
 	Buffer::References::iterator end = refs.end();
 	for (; it != end; ++it) {
-		buffer().undo().recordUndo(it->second);
+		buffer().undo().recordUndo(CursorData(it->second));
 		if (it->first->lyxCode() == MATH_REF_CODE) {
 			InsetMathRef * mi = it->first->asInsetMath()->asRefInset();
 			mi->changeTarget(new_label);