[QUESTION] Some confused situations in Writer UNDO/REDO

[Fan Zheng <zh...@gmail.com>]

Hi, all:

Now I am working on a bugzilla issue 121897, which seems a writer undo/redo
corresponding issue. Inside there are some questions I met, which may need
help from the experts.

Here is background:

There are some refine work in SW undo/redo mechanism for integrating into
the SfxUndoManager framework. Inside the implementation,
SwUndoInserts::UndoImpl, which is the detailed undo method for inserting,
will drop all the hints inside the current SwTxtNode, and then retrieve
them back via the SwHistory::TmpRollback. Similar design also exist in
SwUndoDelete::UndoImpl (Similarly the detailed undo method for deleting),
but for some unknown reason (is there do exist some), the using hints clear
methods are different inside above 2 undo functions. For SwUndoInserts,
the SwTxtNode::RstAttr is used, and for SwUndoDelete,
the SwTxtNode::ClearSwpHintsArr is used. In general, the difference of said
2 hints clearing method is that, RstAttr will ignore the tox mark index and
ClearSwpHintsArr will ignore the fly content when clearing hints.

And such difference makes the phenomenon of i121897 happens: the former tox
mark index is kept when clearing and a new one inserted later from
SwHistory;

So my questions are:

1. Why the undo of inserting use a different clear method from the undo of
deleting? Should we make them identical or, refine the RstAttr for certain
usage in undo inserting?
2. As we see, the whole hints inside the text node will be cleared, even if
the actions should be undone are related to part of the text node. At it is
not a good performance design. Is there any certain reason for doing like
that?

Thanks for your help!

Re: [QUESTION] Some confused situations in Writer UNDO/REDO

[Fan Zheng <zh...@gmail.com>]

2013/5/16 Oliver-Rainer Wittmann <or...@googlemail.com>

> Hi,
>
>
> On 14.05.2013 10:48, Fan Zheng wrote:
>
>> Hi, all:
>>
>> Now I am working on a bugzilla issue 121897, which seems a writer
>> undo/redo
>> corresponding issue. Inside there are some questions I met, which may need
>> help from the experts.
>>
>> Here is background:
>>
>> There are some refine work in SW undo/redo mechanism for integrating into
>> the SfxUndoManager framework. Inside the implementation,
>> SwUndoInserts::UndoImpl, which is the detailed undo method for inserting,
>> will drop all the hints inside the current SwTxtNode, and then retrieve
>> them back via the SwHistory::TmpRollback. Similar design also exist in
>> SwUndoDelete::UndoImpl (Similarly the detailed undo method for deleting),
>> but for some unknown reason (is there do exist some), the using hints
>> clear
>> methods are different inside above 2 undo functions. For SwUndoInserts,
>> the SwTxtNode::RstAttr is used, and for SwUndoDelete,
>> the SwTxtNode::ClearSwpHintsArr is used. In general, the difference of
>> said
>> 2 hints clearing method is that, RstAttr will ignore the tox mark index
>> and
>> ClearSwpHintsArr will ignore the fly content when clearing hints.
>>
>> And such difference makes the phenomenon of i121897 happens: the former
>> tox
>> mark index is kept when clearing and a new one inserted later from
>> SwHistory;
>>
>> So my questions are:
>>
>> 1. Why the undo of inserting use a different clear method from the undo of
>> deleting? Should we make them identical or, refine the RstAttr for certain
>> usage in undo inserting?
>>
>
>
> I am not sure about the existence of two such methods, but I assume that
> the corresponding code had evolved over the last decade and nobody took the
> responsibility to clean it up.
>
> I propose unify the functionality in one method.
>

I am not sure whether it is workable if we decide to simply drop any of
them, for the RstAttr could give more parameters, for more specified
conditions of hints clearing, for example the the clear index ranges and
the hint attributes type. So such unify you mentioned should be
more flexible then they were.


> For a release after AOO 4.0 we should investigate in detail all usages of
> the two existing methods, define certain test cases for these usages and
> then unify the functionality in one method. The most important step here is
> to define the test cases.


I agree with you.

>
>
>  2. As we see, the whole hints inside the text node will be cleared, even
>> if
>> the actions should be undone are related to part of the text node. At it
>> is
>> not a good performance design. Is there any certain reason for doing like
>> that?
>>
>
> I do not know.
> Do you know, if this came in by the SW undo/redo refactoring?
>

No, at least the totally hints clearing stuff exist in
SwUndoDelete::Undo(...) before the sw undo/redo refactoring.  Which exists
maybe years, maybe even older than me :)

The former OOo code repository is still available at
> http://hg.services.openoffice.**org/ <http://hg.services.openoffice.org/>.
> May be a short look at its trunk (namely DEV300) and the corresponding log
> would give some insight. Even if it reveals that this 'complete clear' had
> been made since years.
>

>> Thanks for your help!
>>
>>
> You are welcome.
>
> A question from my side:
> Does defect 121897 occurs before the SW undo/redo refactoring had been
> made?


No, it is a regression defect from SW undo/redo refectoring in my personal
view.


>


>
> Best regards, Oliver.
>
> ------------------------------**------------------------------**---------
> To unsubscribe, e-mail: dev-unsubscribe@openoffice.**apache.org<de...@openoffice.apache.org>
> For additional commands, e-mail: dev-help@openoffice.apache.org
>
>

Re: [QUESTION] Some confused situations in Writer UNDO/REDO

[Oliver-Rainer Wittmann <or...@googlemail.com>]

Hi,

On 14.05.2013 10:48, Fan Zheng wrote:
> Hi, all:
>
> Now I am working on a bugzilla issue 121897, which seems a writer undo/redo
> corresponding issue. Inside there are some questions I met, which may need
> help from the experts.
>
> Here is background:
>
> There are some refine work in SW undo/redo mechanism for integrating into
> the SfxUndoManager framework. Inside the implementation,
> SwUndoInserts::UndoImpl, which is the detailed undo method for inserting,
> will drop all the hints inside the current SwTxtNode, and then retrieve
> them back via the SwHistory::TmpRollback. Similar design also exist in
> SwUndoDelete::UndoImpl (Similarly the detailed undo method for deleting),
> but for some unknown reason (is there do exist some), the using hints clear
> methods are different inside above 2 undo functions. For SwUndoInserts,
> the SwTxtNode::RstAttr is used, and for SwUndoDelete,
> the SwTxtNode::ClearSwpHintsArr is used. In general, the difference of said
> 2 hints clearing method is that, RstAttr will ignore the tox mark index and
> ClearSwpHintsArr will ignore the fly content when clearing hints.
>
> And such difference makes the phenomenon of i121897 happens: the former tox
> mark index is kept when clearing and a new one inserted later from
> SwHistory;
>
> So my questions are:
>
> 1. Why the undo of inserting use a different clear method from the undo of
> deleting? Should we make them identical or, refine the RstAttr for certain
> usage in undo inserting?


I am not sure about the existence of two such methods, but I assume that 
the corresponding code had evolved over the last decade and nobody took 
the responsibility to clean it up.

I propose unify the functionality in one method.
For a release after AOO 4.0 we should investigate in detail all usages 
of the two existing methods, define certain test cases for these usages 
and then unify the functionality in one method. The most important step 
here is to define the test cases.

> 2. As we see, the whole hints inside the text node will be cleared, even if
> the actions should be undone are related to part of the text node. At it is
> not a good performance design. Is there any certain reason for doing like
> that?

I do not know.
Do you know, if this came in by the SW undo/redo refactoring?
The former OOo code repository is still available at 
http://hg.services.openoffice.org/. May be a short look at its trunk 
(namely DEV300) and the corresponding log would give some insight. Even 
if it reveals that this 'complete clear' had been made since years.

>
> Thanks for your help!
>

You are welcome.

A question from my side:
Does defect 121897 occurs before the SW undo/redo refactoring had been made?


Best regards, Oliver.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@openoffice.apache.org
For additional commands, e-mail: dev-help@openoffice.apache.org