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
pgpWl1JAIhjwM.pgp
Description: PGP signature
_______________________________________________ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
