Apache OpenOffice (AOO) Bugzilla – Full Text Issue Listing
|Summary:||UNDO for insert frame moves cursor position to top of document|
|Product:||Writer||Reporter:||Rainer Bielefeld <rainerbielefeld_ooo_qa>|
|Status:||CLOSED FIXED||QA Contact:||issues@sw <issues>|
|Priority:||P3||CC:||chengxiuzhi, cno, issues, liujiaxiang, liuyu, peter.junge, volker|
|Issue Type:||DEFECT||Latest Confirmation in:||---|
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 firstname.lastname@example.org [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 <ok> 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 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 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... Problem: 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. Reason: 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 readjusted. 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 11 liuyu 2007-07-16 03:45:11 UTC
liuyu->ama: 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 http://wiki.services.openoffice.org/wiki/Target_3x
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 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 19 liuyu 2007-10-11 10:09:11 UTC
liuyu->ama: 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 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 SwIndexReg). Please have look at my attachment (id=48547) from Sep 27 and at our Writer Wiki http://wiki.services.openoffice.org/wiki/Writer_Core_And_Layout#Indexes_and_Positions
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 undobj.hxx doclay.cxx undobj1.cxx shellio.cxx unins.cxx untblk.cxx
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.