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

[Thorsten Behrens]

Marco wrote:
> It seems that I have passed the final evaluation!! :)
>
Indeed - congrats, well done! :)

> 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, 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. :)

> @@ -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?

> 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.

> @@ -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". ;)

Otherwise, looks good - thanks for the fixing! :)

Cheers,

-- Thorsten
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/libreoffice/attachments/20110829/35d753fe/attachment.pgp>