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
Attachment:
pgpooSYRru6jC.pgp
Description: PGP signature