[Libreoffice] Undo Redo Re: [REVIEW] Re: SmNodeToTextVisitor Fixes

[Luke Dixon]

Hi Jonas,

On Sun, 2011-02-13 at 21:44 +0100, Jonas Finnemann Jensen wrote:

> It would be major step to get that issue fixed... And you're going the
> right way, but your patch doesn't work if the formula is written in
> multiple lines...
> try, writing the formula 3+3+3 \n \n \n \n +4+4 (replace \n by line
> break not NEWLINE but whitespace linebreak).
> Then in visual editor delete +3, go into the command editor again and
> back into the visual editor and notice that it now says 3+3+4+4+4+4.
> The command editor will say  { 3 + 3 + 4 + 4 }  \n \n \n \n +4+4,
> because the SetText(0, formula) only replaced the first paragraph...
> 

Ah, I guess I got a bit too excited too quickly. I should have known
better than to think it was that easy. :)

> 
> What I understand from editeng_8.cxx line 1621 (see [1]) is that the
> number you pass in setText(0, formula), is the paragraph to be
> replaced by the text...
> The method signature is EditEngine::SetText( sal_uInt16 nPara, const
> XubString& rTxt ), and it calls SelectParagraph(nPara)........
> At least that's what I can decrypt from the sources... (I think I've
> seen ROT13 variants that was easier to read).
> 

Yeah, I saw nPara, and thought about it for a couple of seconds, figured
that since SmNodeToTextVisitor puts it all on one line it would be okay.
I thought that through badly :(

> 
> I have no idea how to solve this... Maybe we can
> call EditEngine::UndoActionStart(USHORT nId) (see [2])
> and EditEngine::UndoActionEnd(USHORT nId), before and after the called
> to EditEngine::SetText(formula)...
> But in that case I don't know if we can pass any value as nId, the
> existing ones are defined in [3] (line 64).
> The only place I can see EDITUNDO_INSERT used is to call
> UndoActionStart/End and in one switch case where labels are
> generated...
> 

I had a very bad solution that seemed to work but made the unit tests
not work.
I replaced SetText with:
    SmGetActiveView()->GetEditWindow()->SelectAll();
    SmGetActiveView()->GetEditWindow()->InsertText(formula);

but it would really need to be done properly.

> 
> We can probably enable the visual editor as an non-default editing
> mode (i.e. activated from toolbar) once the formatting issue is
> addressed... Then when we've integrated it properly, so that users of
> the command editor doesn't feel any uncomfort, we can make it
> default...
> But as long as it discards formatting information (font, bold, color,
> alignment, etc) we probably shouldn't enable it by default... Users
> hate regressions, especially if it eats all their formatting
> information without asking nicely first :)
> 

Oh yeah, forgot about the formatting. Yes, I agree. I guess I just got a
little excited.

I'm very sorry for bothering you with solutions that don't work.
Thanks for putting up with me. :)

Regards,
Luke