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

hi Luke,

I should have known better than to think it was that easy. :)

I think it ought to be that easy... More documentation would speedup
development and reduce bugs dramatically...
(Maybe we should write a few doxygen comments for EditEngine, if we figure
out how it works).

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);

Why is this a bad solution ?
Any idea, why this doesn't work with the unit tests ?
It seems like it might be a fairly good solution... Unless undo/redo manages
selections too... So that this becomes two steps ?

I'm very sorry for bothering you with solutions that don't work.
>
Don't be, it also happens to me... That's why the visual formula editor had
bugs to begin with, and it's no unlikely that we'll find more :)
Besides if we don't propose patches, or partially working fixes... and
discuss these we'll never get undo integration to work properly.

--
Regards Jonas Finnemann Jensen.


On Mon, Feb 14, 2011 at 01:13, Luke Dixon <6b8b4567@gmail.com> wrote:

> 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