Re: [Libreoffice] [GSoC 2011][svgexport][PATCH] Fixed a buf in the JavaScript animation engine

On Mon, 29 Aug 2011 10:04:55 +0200, Thorsten Behrens  
<thb@documentfoundation.org> wrote:

> Marco wrote:

skip

> > I attached a new patch set based on the master branch, as it was on
> > Monday July 7, that includes a bug fix: in my haste I missed to
> > make animations on the starting slide work properly.
> Since all patches from your tarbomb except
> 0019-Fixed-a-bug-in-the-JavaScript-animation-engi-filters.patch are
> pushed,

Do you mean pushed on OneGit master branch ?


>         it's easier to just attach that - permits convenient inline
> commenting ... ;)
>
> > diff --git a/filter/source/svg/svgscript.hxx  
> > b/filter/source/svg/svgscript.hxx
> > index 7cbed89..01748e7 100644
> > --- a/filter/source/svg/svgscript.hxx
> > +++ b/filter/source/svg/svgscript.hxx
> > @@ -35,7 +35,7 @@ static const char aSVGScript0[] =
> >  "<![CDATA[\n\
> >  \n\
> >       /*****  * * * * * * * * * * * * * * * * * * * * * * * * * * * * *  
> > * * *  *****\n\
> > -      *                      - Presentation Engine v5.0.1  
> > -                       *\n\
> > +      *                      - Presentation Engine v5.0.2  
> > -                       *\n\
> >        *****  * * * * * * * * * * * * * * * * * * * * * * * * * * * * *  
> > * * *  *****/\n\
> that looks like mostly noise to me - I can get history from git just
> as well. :)

Well, I understand this one and your other comments, but I think you should
take into account that all the script is embedded into the svg document.
If a user incurs into a problem with the animation engine and mail his/her
document to us we can get immediately which script release is.


> > @@ -48,7 +48,7 @@ static const char aSVGScript0[] =
> >  \n\
> >       /*****   
> > ******************************************************************\n\
> >        *\n\
> > -      *   DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE  
> > HEADER.\n\
> > +      *   DO NOT ALTER OR REMOVE COPYRIGHT NOTICES.\n\
> >        *\n\
> I feel very uncomfortable with changing copyright notices on a whim
> - especially with a patch that says nothing about it in the commit
> notice. Could you please expand on why this is necessary?

Again, the script and its legal notice are embedded into a svg
document, so it seems that it is meaningless the fragment
"OR THIS FILE HEADER". As far as the fact I didn't point out that
in the commit notice, well it didn't seem to me a so big change
(moreover I did not modified the legal notice itself).


> > static const char aSVGScript4[] =
> >  "\
> > +         else if( nIndex > nMax )\n\
> > +             return nMax;\n\
> >           else\n\
> >               return nIndex;\n\
> and other places - the different splitting up into pieces looks
> rather random - any chance to have that fixed? It makes patch
> review unnecessarily hard. In this case, I have to check lines and
> lines of (seemingly) cruft - which really does not help to get a
> quick review done, and may lead to your patch being postponed
> because I get interrupted with other stuff.

Almost a month ago I started using a self-made python script for
splitting automatically the JavaScript code into pieces, and,
after the boost in size it has been very helpful.
Lately I enhanced this python script in several way.
At present it is able to handle any '\' character found into
the JavaScript code that obviously if it is not substituted with
a double '\' would cause compilation errors.
Moreover it stripes out any not mandatory comment (that is
all except legal notices) in order to reduce the amount of lines
to be embedded into the final svg document.

From my point of view the C++ header is not anymore the right
place where to look for JavaScript presentation engine changes.
I think that the best solution is to include into the commit the
JavaScript code file itself (and the python script too).


> > @@ -1848,7 +1854,7 @@ static const char aSVGScript8[] =
> >               theSlideIndexPage.show();\n\
> >               currentMode = INDEX_MODE;\n\
> >           }\n\
> > -         else if (currentMode == INDEX_MODE)\n\
> > +         else if( currentMode == INDEX_MODE )\n\
> >           {\n\
> >               theSlideIndexPage.hide();\n\
> >               var nNewSlide = theSlideIndexPage.selectedSlideIndex;\n\
> No prob with making indentation/whitespace consistent - but much
> more helpful when reviewing, to have separate patches for that, with
> a helpful comment mentioning "whitespace cleanup". ;)

Ok, I'll pay more attention, now on. As you have probably noted
I have not yet got habit to all the details involved in contributing
in a so large project as LibreOffice.

Cheers,
-- Marco



--
Using Opera's revolutionary e-mail client: http://www.opera.com/mail/