MRU->AMA: as described - UNDO on sorting text having an as-char-graphic in
Redlines -> crash

*** Issue 97523 has been marked as a duplicate of this issue. ***

Due to our workload and resources (development as well as QA) I've to retarget
this issue to OOo3.2.

Created attachment 64333 [details]
patch file

Please have a check about this patch,thanks in advance.

ama->mst: Could you please take care of this patch?

hi majun51,

i have had a look at your patch, and tried it out.
sorry that it took so long, but i've had no time before, and the redlining code
is rather... complicated.

(aside: the patch is inverted, because you put the parameters to diff in the
wrong order; just a minor inconvenience)

Your patch unfortunately introduces a local variable nOffset; the class
SwUndoRedlineSort already has a member of the same name. This fails to build
with the Solaris compiler. Please rename your variable.

So, the problem is that the sort with redline on is implemented as a copy of the
selected nodes, followed by sorting of the original nodes, and marking the
inserted copy of the nodes (which are in original order) as a delete redline.
When the copied nodes contain frames, then these frames are also copied.
Because of the awful way frames are represented in the writer model, namely, a
section in a special area of the nodes array that precedes the body of the
document, this changes the offsets of the nodes in the sorted text.

It seems to me that the patch actually fixes the hang which is described in this
issue, by remembering the previous node (before the selection), and updating the
node indices stored in the undo object to account for the removed frames on UNDO
(and also, the inserted frames, on REDO).

Unfortunately, there is also another problem.
In order to reproduce this problem, you need to have the following:
- selection with paragraphs with a frame, as described above
- a DELETE redline in the selection
- the menu option Edit->Changes->Record must be ON
- the menu option Edit->Changes->Show must be OFF

Without the patch, on UNDO, the document content seems corrupted:
the delete redline with the copy of the nodes that was inserted by the sort is
not removed.
With the patch, on UNDO, it crashes when dereferencing pCNd->Len(),
because pCNd is 0.

The reason for this is the constructor of SwUndoRedline, and its Undo/Redo methods.
The code checks if there are hidden redlines in the selection, and calculates an
offset.
Apparently something is going wrong there and the offset is not correct.
(Also, i do not fully understand why there is a call to pRedlUndo->SetValues in
SwDoc::SortText)

Probably the easiest way to solve this problem would be to set the redline mode
to show all redlines in the SwDoc::SortText method (and reset it before
returning), as is done for other SwDoc methods already.

Alternatively, you could try to find out where the accounting of
SwUndoRedlineSort::nOffset goes wrong.

hi mst,
Thank you for checking!
sorry for the inverted patch and stupid mistakes!
i can reproduce the problem you'd pointed out. It's my fault in the patch that
it didn't take hidden redline into account. Obviously hidden redline changes 
the offsets of nodes in the sorted text too. 
Thank you very much for your suggestions, both are very reasonable, of course
the former is an easiest way.  I'll rework on this issue soon after spending our
National Day holiday. 

Created attachment 65328 [details]
patch 2 for this issue

hi mst,
For this patch, 
  i) added two members to record the original char index for selection text.
  ii)adjusted code for hidden redline: if there was hide redline, show it up first.
so it can work for redlining under both modes(show and hide). Please have a look
at and remind me if anything missed, thanks.

By the way, i find another big problem for SortText(): Redlines in texts which
are in sorting paragraph but not in cursor selection area lose after sorting and
after undo. Redlines are DELETE type redlines will lose content under hidden
mode. It is mainly because sorting is for full paragraph, but only cursor
selecting range is saved in SwUndoRedlineSort.

hi majun51,

so the code that tries to display the redline in SwUndoRedlineSort::_Undo calls
Show(), which is actually Show(0), which does not actually do anything at all?
ouch... clearly this code has never been tested.

Thanks for pointing out that redlines that are in the start/end paragraph but
not in the selection get lost; i had not noticed that.
I believe it should be sufficient to just expand the PaM in SwDoc::SortText so
it selects the whole start and end paragraphs, because, after all, this function
sorts whole paragraphs.
(probably best to do this after SortText has checked that all nodes are text nodes)
Furthermore, with this change, you should not need the new members for storing
the selection start/end index, because these indices are then always 0 and
pEndNode->Len().
That should be much simpler, and we do not complicate the undo object with this.

Can you try out whether this works?

Created attachment 65689 [details]
patch 3 for this issue

Created attachment 65693 [details]
patch 3: the former patch forgot to erase one parameter added in Ctor for storing the selection start/end index

hi mst,
Sorry for not replying in time. Last two weeks i was busy on other tasks.
For you suggestion,
i)Yes, the call Show(0) does nothing but Show(1) do MoveFromSection().
ii)Yes, this function sorts whole paragraphs and according you suggestion that
just expand the PaM seems work well.
iii)OK, new members for storing the selection start/end index seems
needless.(They are needless for sort function,they may be need just when user
want to see their seletion range in UI after undo sort feature)
 
For this patch, just expand the PaM to whole seleted paragraphs rather than the
selection range.
Please check and point out if anything wrong or missed, thanks very much.

hi majun51,

no matter about the delay, i was on vacation anyway :)

> ii)Yes, this function sorts whole paragraphs and according you suggestion that
> just expand the PaM seems work well.

good to hear!

> iii)OK, new members for storing the selection start/end index seems
> needless.(They are needless for sort function,they may be need just when user
> want to see their seletion range in UI after undo sort feature)

hmm... i am not sure what the selection after Undo should be:
it could be either the selection that was changed by the Undo operation, or the
selection that was originally done by the user.
For most actions, these two would be the same, but Sort seems to be an exception
here.

But unfortunately i have found another problem with your patch...
Actually it was already in your second patch, but i did not notice.
The problem is the Move operation that actually does the sort in SortText():
it reorders the nodes (obviously), but it does not modify the rPaM.
So the order of the nodes changes, but the rPaM is still registered at the same
nodes as before:
these are not necessarily the nodes at the start and end of the sorted range!
As a consequence, after the move the rPaM should not be used any more, like in
the SetValues(rPaM) in your patch.

Sorry for not noticing this earlier, in my tests i must have used already-sorted
text.
(did i already complain that the redlining code is too complicated?)

But it seems to me that when adding the INSERT redline, the pRedlPam should have
the right values...
Can you try moving the SetValues a couple lines down, and use pRedlPam; i hope
that fixes the problem.

Created attachment 65721 [details]
patch 4

hi mst,

Thanks very much to point out the question about SetValues(rPaM) in my patch.
You are right that the rPaM became something invalid after Move operation(let me
just call it misorder with start/end nContent index,but the start/end nNode
index is still correct in rPaM) 
The call SetValues(rPaM) is used to update the Range in pUndo by rPaM in time(
it should be called after inserting delete redline mainly because it changes the
order of Nodes under hidden redlining mode )

The pRedlPaM is used in the AppendRedline(...REDLINE_DELETE...) which represents
the range of delete redline, and then used in the
AppendRedline(...REDLINE_INSERT...). But before
AppendRedline(...REDLINE_INSERT...) pRedPaM was adjusted a little so it can
represents the range of insert redline:
But it only adjust the pPoint to the start of the range of insert redline(that
is to say,its pMark is still at the start of range of delete redline). 
We should adjust its pMark member to be at the end of the range of insert
redline,it's really one node offset!(i think delete redline is just followed
insert redline) 

Really, i do think the redlining code is too complicated and there are conflict
between undo and redlining...:(


so in this patch:
 - moving the SetValues much lines down and use pRedlPam as you have suggested. 

Please check and point out if anything wrong or missed, thank you very much.

hi majun51,

hmm, yes, it seems adjusting the pRedlPam mark a little is necessary, otherwise
it crashes on UNDO!
Now i do not see any problems any more.

Committed your patch on CWS sw33bf01:
http://hg.services.openoffice.org/hg/cws/sw33bf01/rev/8f8f3412c2af

Thanks for your patch!

I have made one further adjustment to SwDoc::SortText(): reorder the creation of
DELETE redline a little, to prevent an ASSERT: "Es sind noch Indizies
angemeldet" (there are indices registered at ~SwIndexReg()).
(This assertion occurs when Changes->Show is disabled, on creation of the DELETE
redline.)

http://hg.services.openoffice.org/hg/cws/sw33bf01/rev/2b3472d85546

set target

please verify

Verified fix in CWS sw33bf01.

Checked fix in DEV300m68.