Issue 59025

Summary: UNDO for insert frame moves cursor position to top of document
Product: Writer Reporter: Rainer Bielefeld <rainerbielefeld_ooo_qa>
Component: uiAssignee: michael.ruess
Status: CLOSED FIXED QA Contact: issues@sw <issues>
Severity: Trivial    
Priority: P3 CC: chengxiuzhi, cno, issues, liujiaxiang, liuyu, peter.junge, volker
Version: OOo 2.0.1Keywords: oooqa
Target Milestone: ---   
Hardware: PC   
OS: Windows XP   
Issue Type: DEFECT Latest Confirmation in: ---
Developer Difficulty: ---
Description Flags
test document
the patch file
patch file
Patched patch ;-) none

Description Rainer Bielefeld 2005-12-07 13:58:49 UTC
With “2.0.1 RC2 German version WIN XP: [680m143(Build8986)]†I checked a problem
disussed in [OOo 2.01RC2 Rückgängig]

Steps to reproduce:
1. Open attached test document 'undoframeproblem.odt'
2. go to end of document with <cntrl>+<End>
3. Menu "Insert - Frame" for inserting a standard frame without 
   any change of properties
4. Press undo button in toolbar (as long as frame still is marked as "selected")
   expected: frame should be removed and cursor remain at last position
   actual: cursor position moves to top of document after frame has disappeared

For me reproducible nearby 100% with 2.0.1RC2, but also with 2.0 (as it seems
only 30%, but difference might be only because of very few tests)
Comment 1 Rainer Bielefeld 2005-12-07 14:00:09 UTC
Created attachment 32169 [details]
test document
Comment 2 michael.ruess 2005-12-07 14:16:26 UTC
MRU->HBRINKM: new document, insert some page breaks, insert a frame, Undo ->
cursor jumps to first position of document
Comment 3 lohmaier 2005-12-07 17:12:04 UTC
very similar to issue 18014
Comment 4 g.marxen 2005-12-09 16:47:07 UTC
The bug is not completely fixed. 

If the frame in Writer (OOo 2.0.1 RC3 englisch version under Win2K) is selected,
the cursor jumps to the beginning of the first page:

"Insert, Frame, OK", then imediately click on the undo-button: jump to first page. 

"Insert, Frame, OK", click in the text (deselecting the frame), then click on
the undo-button, cursor stays where he shouldt to stay. 
Comment 5 tsealex 2006-01-27 05:10:21 UTC
Successfully reproduced the bug on OOo 2.0.1 with WinXP SP2.
Reproducing the bug as follow:

1. Create a new document in Writer
2. Insert page break by pressing Ctrl-Enter (the cursor should now staying in
the beginning position of the 2nd page)
3. Go to "Insert" -> "Frame" ; then press "Ok"
4. Without clicking anywhere else, press "Undo"

Note that the cursor will go to the beginning position of the document (first page).

Follow-up Testing:
1. 1. Create a new document in Writer
2. Insert page break by pressing Ctrl-Enter (the cursor should now staying in
the beginning position of the 2nd page)
3. Type "HellowWorld" and move the cursor to between "wW"
4. Go to "Insert" -> "Frame" ; then press "Ok"
5. Without clicking anywhere else, press "Undo"

The cursor will again move to the beginning position of the document.  
The curosr will move to the beginning position of the document as long as you
immediately press "Undo" follows by inserting a frame, no matter where the
cursor at.
Comment 6 openoffice 2006-07-07 10:41:26 UTC
changed target to 2.x
Comment 7 openoffice 2006-09-01 16:38:55 UTC
responsibility for undo changed to ama
Comment 8 michael.ruess 2006-10-26 08:53:02 UTC
*** Issue 70860 has been marked as a duplicate of this issue. ***
Comment 9 andreas.martens 2007-07-09 14:33:09 UTC
ama->liuyu: Maybe the following findings are helpful...

1. Create new document
2. Insert some text
3. Put the cursor to the end of the text.
4. Insert a text frame.
5. Undo
=> The cursor jumps to the top of the document.

When Undo is performed, the current cursor is placed in the text frame. This
text frame vanishes during the Undo action, so the cursor position has to be
Current code:
SwUndoInsLayFmt::Undo(..) calls
SwUndo::RemoveIdxFromSection(..) calls
SwDoc::CorrAbs(..), this methods sets the current cursor from the vanishing node
to the first SwEndNode of the document's nodes array, but this is not a valid
cursor position.
Later on 
SwCrsrShell::EndAction(..) calls
SwCrsrShell::UpdateCrsr(..) calls
SwCrsrShell::ClearUpCrsr(..), this method recognizes the invalid cursor position
and sets it to the top of the document.
Comment 10 liuyu 2007-07-16 03:43:37 UTC
Created attachment 46814 [details]
the patch file
Comment 11 liuyu 2007-07-16 03:45:11 UTC
I have submitted the patch file, please have a look at it.
Comment 12 andreas.martens 2007-07-16 14:16:03 UTC
Yes, your patch solves this issue for the most use cases and could be integrated
without changes. But I've some ideas how you could make it even better.

1. I said "most" use cases. But if you create more than one view to your
document (Window/New_window) your patch will work for one view only. This is a
little nitpicking, because this is not very usual and actually your patch
improves the situation even for two views.
2. One important goal for me is to make our code more maintainable. So I'm
trying to reduce the dependencies bewtween different parts of our code. From
that point of view I would prefer not to use (and the new include of) the
SwCrsrShell in undo code.

How can we stay with your idea (remember the cursor position at the undo object
and use this if needed) and solve 1. and 2.?
What about the use of a node index (or a pair node index/content index) instead
of the document coordinates? In SwDoc::_MakeFlySelection(..) there is a
parameter rAnchPos which can be used. You could remember the node index of this
parameter in undo (only the index, not the node itself). This index can become a
new optional parameter for SwUndo::RemoveIdxFromSection(..) and can be used to
construct the aPos object. This should work for all views.
What do you think?
Comment 13 cno 2007-07-22 15:35:09 UTC
Comment 14 Martin Hollmichel 2007-09-10 13:52:56 UTC
move to 3.x accroding
Comment 15 andreas.martens 2007-09-13 14:07:31 UTC
We have to change SwUndo::RemoveIdxFromSection(..) somehow. At the moment this
function creates a position aPos at aNodes.GetEndOfPostIts(). This is not a
rational position for any cursor. In the following all cursors (from every
view/shell) will be set to this "position" if they are in the deleted area (e.g.
inside our removed fly frame). Later on this position will be corrected to the
first text position in the document. If SwUndo::RemoveIdxFromSection() would
deliver a more sensible position aPos, no cursor would jump to the start of
document. But which position should be used from SwUndo::RemoveIdxFromSection()
instead of "GetEndOfPostIts"? There are a lot of opportunities:
SwUndo::RemoveIdxFromSection() could be get another parameter with a position or
it could call a new virtual method which is overwritten by SwUndoInsLayFmt.
Comment 16 andreas.martens 2007-09-27 13:16:21 UTC
Created attachment 48547 [details]
Comment 17 andreas.martens 2007-09-27 13:20:58 UTC
ama->liuyu: I've just attached a patch which sets the cursor to the end of the
document. This is of course not the solution but it shows that changing of
SwUndo::RemoveIdxFromSection(..) is a possible way to go.
At the moment the method is *not* virtual. If you are planning to overload this
function don't forget to make it virtual in the base class as well.
Hope that helps.
Comment 18 liuyu 2007-10-11 10:08:09 UTC
Created attachment 48831 [details]
Comment 19 liuyu 2007-10-11 10:09:11 UTC
I have submitted the patch file, please have a look at it.
Comment 20 andreas.martens 2007-10-12 11:48:41 UTC
I had a look at the patch and I have some questions about it. Let us discuss
this via IRC, I'll send you an email.. 
Comment 21 liuyu 2007-10-25 07:39:51 UTC
Created attachment 49138 [details]
Comment 22 liuyu 2007-10-25 07:48:20 UTC
Liuyu->ama: I have submitted the patch file. I changed the code according to 
the result we discussed last time. Recently I was busy with other tasks, so 
this patch is somewhat late.
Comment 23 andreas.martens 2007-10-25 10:42:12 UTC
ama->liuyu: I have two remarks to your patch.
1. For the construction of the SwUndoInsLayFmt you get the new needed ULONGs
from two different members?! The SwNodeIndex-ULONG you get from rAnchPos but the
SwIndex-ULONG you get from GetEditShell->GetCrsr().. You better use rAnchPos for
the SwIndex-ULONG as well. Even at that moment
GetEditShell()->GetCrsr()->GetPoint() seems to be identical to rAnchPos, it
makes the code more at least more readable.
2. When you create a SwPosition again from those two ULONGs, you need a SwNode
and a SwIndex again. How you get the SwNode is oikay, but you should not create
a SwIndexReg on the heap and use this for SwIndex. (BTW: this is a memory leak
because this object will never be destroyed). If a SwPosition is created with a
SwNode and a SwIndex, there is a "must": if the SwNode is a SwTxtNode, the
SwIndex has to be registered in this SwTxtNode. (Remember: a SwTxtNode is a
Please have look at my attachment (id=48547) from Sep 27 and at our Writer Wiki
Comment 24 liuyu 2007-11-02 07:08:38 UTC
Created attachment 49375 [details]
patch file
Comment 25 liuyu 2007-11-02 08:20:10 UTC
liuyu->ama: I have read the wiki about the Indexes and Positions. Thank you 
very much, it is very helpful.
Comment 26 andreas.martens 2007-11-06 15:20:53 UTC
Created attachment 49468 [details]
Patched patch ;-)
Comment 27 andreas.martens 2007-11-06 15:24:46 UTC
ama->liuyu: Even I have some remarks to your patch (please have a look into my
last attachment), this issue can be regarded as solved!
I will do some minor changes and check your patch in into CWS sw8u10bf01 for
target OOo2.4. Thank you for your work!
Comment 28 andreas.martens 2007-11-06 15:33:46 UTC
Fixed in CWS sw8u10bf01
Comment 29 liuyu 2007-11-07 03:07:08 UTC
Liuyu->ama: I have read your remarks in the last attachment, I think your 
changes are exactly better. Thank you very much for your guidance.
I think I could start with an other issue.
Comment 30 andreas.martens 2007-11-20 08:39:10 UTC
Ready for QA.
Comment 31 michael.ruess 2007-11-27 13:58:42 UTC
Verified fix in CWS sw8u10bf01.
Comment 32 michael.ruess 2008-01-11 13:44:38 UTC
Checked fix in OO 2.4 dev build 680m241.