DO NOT REPLY [Bug 49801] Region-Body Column balancing incorrect if content is table with header
https://issues.apache.org/bugzilla/show_bug.cgi?id=49801 alpa...@gmail.com changed: What|Removed |Added CC||alpa...@gmail.com -- Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug.
Re: Assertions in junit tests [was: Re: Solving FindBugs issue]
On 22 Feb 2011, at 08:50, Simon Pepping wrote: On Mon, Feb 21, 2011 at 08:28:33PM +0100, Andreas Delmelle wrote: I saw one exclusion --unconfirmed cast-- that would seem to stem from my recent refactoring in the BlockStackingLMs. Not sure why an exclusion was chosen here, but adding an assert statement in the code avoids the warning as well *and* has the benefit of being visible exactly at the spot in the code where it applies. Seemed like the more proper way to handle this. It would be nice if assertions were checked during the junit tests. However, when I set: junit ... jvm=java -ea/, I get junit.framework.AssertionFailedError errors. How can this be done? Sounds like a good idea. IIC, simply using -ea would enable internal assertions for JUnit's classes as well. You probably want something like -ea:org.apache.fop... to make sure only assertions in FOP's code are taken into account. Regards, Andreas ---
Re: Solving FindBugs issue
On 22 Feb 2011, at 08:34, Simon Pepping wrote: When I build the project or part of it with Eclipse, and run findbugs afterwards (with ant), I get a number of errors. Now I always make a clean compile before running findbugs. I do not understand why Eclipse builds would create findbugs errors where a clean ant build does not. It makes findbugs seem fickle. I seem to have the reverse issue here: the Ant target yielded warnings that the FB plugin for IntelliJ IDEA does not complain about (same FB version, same exclusion file)... Maybe the developers of those plugins were not using FindBugs themselves. ;-) Regards, Andreas ---
Re: Assertions in junit tests [was: Re: Solving FindBugs issue]
On 22 Feb 2011, at 19:04, Andreas Delmelle wrote: On 22 Feb 2011, at 08:50, Simon Pepping wrote: It would be nice if assertions were checked during the junit tests. However, when I set: junit ... jvm=java -ea/, I get junit.framework.AssertionFailedError errors. How can this be done? Sounds like a good idea. IIC, simply using -ea would enable internal assertions for JUnit's classes as well. You probably want something like -ea:org.apache.fop... to make sure only assertions in FOP's code are taken into account. Tried that and failed.. :( One way to enable assertions I found so far, and that seems to work: - use fork=true on the junit target - insert: assertionsenabled //assertions in that target That seems to yield the expected behavior at first glance. Our junit targets are not forked, however. Not sure if that is preferable... Regards, Andreas ---
Re: Solving FindBugs issue
Since the FOP project itself does not use any IDE for builds, then the ANT build process should be considered the standard process for compilation, junit tests, checkstyle tests, findbugs tests, etc. I always run: ant clean junit checkstyle findbugs in order to verify no reported errors before I commit a change. I notice also that the nightly build target does not run all the junit tests. It would be better if it run all of them plus checkstyle and findbugs. G. On Tue, Feb 22, 2011 at 12:34 AM, Simon Pepping spepp...@leverkruid.euwrote: When I build the project or part of it with Eclipse, and run findbugs afterwards (with ant), I get a number of errors. Now I always make a clean compile before running findbugs. I do not understand why Eclipse builds would create findbugs errors where a clean ant build does not. It makes findbugs seem fickle. I just tested it. With 'ant clean findbugs' I get 0 errors and warnings. With eclipse clean and build project, I get 23 low priority warnings. Simon On Mon, Feb 21, 2011 at 12:58:06PM -0700, Glenn Adams wrote: The current trunk shows no warnings during ANT compile. Please make reference to the current trunk/HEAD as 1.0 is published and history at this time. It's a different matter with certain IDEs, e.g., Eclipse, which set their warning levels to a more sensitive level than the ANT build. Although it would be nice to eliminate such additional warnings, it is not as high priority IMO as ensuring that new compile, checkstyle, or findbugs warnings/errors do not appear during ANT builds. At the same time, warnings that do appear should not automatically be excluded without careful, manual review. G. On Mon, Feb 21, 2011 at 12:49 PM, Eric Douglas edoug...@blockhouse.com wrote: I haven't looked at the trunk lately but 1.0 has a ton of warnings, at least in my compile. I don't know how much warnings have changed over the versions. I think it was originally written to compile on Java 1.4 or maybe even 1.3. 1.5 shows thousands of warnings, 1.6 shows more. Some of the warnings are quirky (raw type list?), some are just wasteful (dead code? Local variable never referenced?). If there's code which is actually incorrect logic it needs to be fixed. If there's code which is just incomplete logic, maybe setting something up for a future improvement someone wanted to add, that should be removed and placed in a separate to do list document.
Re: Solving FindBugs issues
On 22/02/11 07:24, Simon Pepping wrote: Not all FOP developers are willing to use findbugs. I hid the findbugs errors as a courtesy to those FOP developers who do use findbugs, so they can check their own code based on a clean slate. I agree that ignoring all the existing issues at the time FindBugs was set up was necessary, but now that FindBugs is in place I don’t think we want to blindly ignore its output any more. Again, some of the issues raised after the recent commits seem valid and ought to be fixed. Can we revert commit 1071912 and re-consider the issues one-by-one before ignoring them? FOP's history has left us with a very large number of existing findbugs errors. It makes no sense to comment on that; it is a fact of life. FOP's code first of all does a good job at being a successful, heavily used FO processor. Only after that comes the question of a clean code base. That said, of course every FOP developer and every FOP user is welcome to evaluate and possibly remedy one or more findbugs errors. For precisely that reason I put comments in the exclusion file, so one can see which errors have been simply hidden and which have been truely evaluated. Simon Thanks, Vincent On Mon, Feb 21, 2011 at 06:15:13PM +, Vincent Hennebert wrote: Hi, If we solve issues raised by FindBugs by listing them in an ignore file, is there still a point to use FindBugs at all? It seems to me that some of those issues deserve to be fixed. They seem to point out genuine problems in the code. Vincent On 18/02/11 08:18, spepp...@apache.org wrote: Author: spepping Date: Fri Feb 18 08:18:04 2011 New Revision: 1071912 URL: http://svn.apache.org/viewvc?rev=1071912view=rev Log: Fixing checkstyle errors and hiding fingbugs errors Modified: xmlgraphics/fop/trunk/findbugs-exclude.xml xmlgraphics/fop/trunk/src/java/org/apache/fop/render/pdf/PDFImageHandlerSVG.java Modified: xmlgraphics/fop/trunk/findbugs-exclude.xml URL: http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/findbugs-exclude.xml?rev=1071912r1=1071911r2=1071912view=diff
Re: Solving FindBugs issues
On 22 Feb 2011, at 20:15, Vincent Hennebert wrote: snip / Can we revert commit 1071912 and re-consider the issues one-by-one before ignoring them? +1 As far as I can see, the raised warnings are really not so challenging that they cannot be addressed right away. Implementing an equals() method, removing unread fields and redundant checks... I'm thinking, all a matter of minutes if you put your mind to it, no? --and so, I caved in started already... Regards, Andreas ---
Re: Solving FindBugs issues
On 22 Feb 2011, at 20:39, Andreas Delmelle wrote: ... --and so, I caved in started already... In PDFFactory, I have some conflicts to work out first, but the fix-up is rather simple there. In the respective methods, eliminate all unnecessary boxing and use explicit (double) casts where appropriate. ColorUtil seems to be somewhat of a can of worms... It's not restricted to the method where the issue was raised, but likely the result of copy-pasting a pattern from a method that was already 'excused'. At any rate, catching plain Exception should really be avoided, IMO, and traded for catching the specific type(s) of exception we know can be thrown in that block of code. In that case, it is completely unnecessary to resort to silly-looking constructs like catching PropertyException just to rethrow it (since the method allows it anyway). If I judge correctly, for some (if not most) of the methods, there seems to be no need for a try-catch block (?) 3 remaining on my end ;-) Regards, Andreas ---
Re: Solving FindBugs issues
On 22 Feb 2011, at 21:50, Andreas Delmelle wrote: ColorUtil seems to be somewhat of a can of worms... snip / If I judge correctly, for some (if not most) of the methods, there seems to be no need for a try-catch block (?) Sorry, forgot RuntimeExceptions, of course, which are converted into PropertyExceptions by said catch-blocks... Even then, explicitly catching RuntimeException as a last resort would seem preferable to plain Exception. That would have avoided my confusion here, at least. ;-) Regards, Andreas ---
DO NOT REPLY [Bug 50723] Incorrect text underlines position for some fonts
https://issues.apache.org/bugzilla/show_bug.cgi?id=50723 --- Comment #4 from Peter Coppens pc.subscripti...@gmail.com 2011-02-22 16:44:59 EST --- Wonder whether anyone of the fop community is willing to share viewpoints on the approach taken in the patch. Seems a fairly safe change at first sight. -- Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug.
Re: svn commit: r1073518 - /xmlgraphics/fop/trunk/src/java/org/apache/fop/util/ColorUtil.java
On 22 Feb 2011, at 22:38, adelme...@apache.org wrote:
Author: adelmelle
Date: Tue Feb 22 21:38:28 2011
New Revision: 1073518
snip /
@@ -402,7 +398,6 @@ public final class ColorUtil {
/* Ask FOP factory to get ColorSpace for the specified ICC
profile source */
if (foUserAgent != null iccProfileSrc != null) {
-assert colorSpace == null;
To revisit my earlier point about assertions:
Note that this is a fine example of how assert should *not* be used, IMO.
It is meant to test for conditions assumed to be true for as far as we
understand/intend the code to work, but for which this 'guarantee' is, strictly
speaking, non-verifiable at compile-time.
If you look 10 lines up, you see that, if iccProfileSrc has been assigned a
value, that means the assignment of colorSpace did not take place, hence it is
redundant to explicitly re-assert this.
It would be the same as hard-coding 'assert true;'.
Regards,
Andreas
---
Re: Assertions in junit tests [was: Re: Solving FindBugs issue]
On 22 Feb 2011, at 19:25, Andreas Delmelle wrote: One way to enable assertions I found so far, and that seems to work: - use fork=true on the junit target - insert: assertionsenabled //assertions in that target That seems to yield the expected behavior at first glance. Our junit targets are not forked, however. Not sure if that is preferable... FWIW, having configured this on the junit-layout-standard target, yields one AssertionError [junit] Testcase: block-container_reference-orientation_bug36391.xml(org.apache.fop.layoutengine.LayoutEngineTestSuite$1): Caused an ERROR [junit] null [junit] java.lang.AssertionError [junit] at org.apache.fop.layoutmgr.BreakingAlgorithm.getLineWidth(BreakingAlgorithm.java:1394) ... For the remainder, all layout tests pass. Regards, Andreas ---
Re: Assertions in junit tests [was: Re: Solving FindBugs issue]
I support enabling assertions by default on junit tests. More testing is a good thing. G. On Tue, Feb 22, 2011 at 3:40 PM, Andreas Delmelle andreas.delme...@telenet.be wrote: On 22 Feb 2011, at 19:25, Andreas Delmelle wrote: One way to enable assertions I found so far, and that seems to work: - use fork=true on the junit target - insert: assertionsenabled //assertions in that target That seems to yield the expected behavior at first glance. Our junit targets are not forked, however. Not sure if that is preferable... FWIW, having configured this on the junit-layout-standard target, yields one AssertionError [junit] Testcase: block-container_reference-orientation_bug36391.xml(org.apache.fop.layoutengine.LayoutEngineTestSuite$1): Caused an ERROR [junit] null [junit] java.lang.AssertionError [junit] at org.apache.fop.layoutmgr.BreakingAlgorithm.getLineWidth(BreakingAlgorithm.java:1394) ... For the remainder, all layout tests pass. Regards, Andreas ---
DO NOT REPLY [Bug 50723] Incorrect text underlines position for some fonts
https://issues.apache.org/bugzilla/show_bug.cgi?id=50723 --- Comment #5 from Andreas L. Delmelle adelme...@apache.org 2011-02-22 18:15:02 EST --- Looking closer, this indeed seems like a fairly inconsequential (apart from fixing the bug, obviously ;-P). Still, endorsing and committing it would seem to require incorporating such a font (hardcoding?) in the test suite... which would either make it dependent on the font being locally available, or we would have to distribute such a font (?) Or is there another way to check this? Anyone? -- Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug.
DO NOT REPLY [Bug 50723] Incorrect text underlines position for some fonts
https://issues.apache.org/bugzilla/show_bug.cgi?id=50723 Andreas L. Delmelle adelme...@apache.org changed: What|Removed |Added Depends on||50483 --- Comment #6 from Andreas L. Delmelle adelme...@apache.org 2011-02-22 19:28:19 EST --- Seems like Mehdi's efforts in bug 50483 may offer an answer to my question. The patch also touches TTFFile and, IIC, adds testing-infrastructure that might be of use here (?) -- Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug.
DO NOT REPLY [Bug 50483] [PATCH] Full font embedding and Subset embedding in Post Script
https://issues.apache.org/bugzilla/show_bug.cgi?id=50483 Andreas L. Delmelle adelme...@apache.org changed: What|Removed |Added Blocks||50723 -- Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug.
Re: Solving FindBugs issues
On Tue, Feb 22, 2011 at 07:15:17PM +, Vincent Hennebert wrote: On 22/02/11 07:24, Simon Pepping wrote: Not all FOP developers are willing to use findbugs. I hid the findbugs errors as a courtesy to those FOP developers who do use findbugs, so they can check their own code based on a clean slate. I agree that ignoring all the existing issues at the time FindBugs was set up was necessary, but now that FindBugs is in place I don’t think we want to blindly ignore its output any more. Again, some of the issues raised after the recent commits seem valid and ought to be fixed. Can we revert commit 1071912 and re-consider the issues one-by-one before ignoring them? Yes, you can. My position is as follows: I do merging of trunk into complexscripts as a service to the Complex Scripts project. I see existing findbugs errors and warnings. I do not have time nor interest to fix those. So I hide them so that others can run findbugs on their code from a clean slate. I add a new, appropriately labeled section to the findbugs exclusion file so that you and others can do as you suggest. I will do this again and again, no matter how many emails are written about the subject. And I would prefer it if no emails about it were written. They bother me and they discourage me to continue doing any services for FOP. It is much more encouraging to see that some of us pick up the findbugs error report and use it to make code improvements. Simon FOP's history has left us with a very large number of existing findbugs errors. It makes no sense to comment on that; it is a fact of life. FOP's code first of all does a good job at being a successful, heavily used FO processor. Only after that comes the question of a clean code base. That said, of course every FOP developer and every FOP user is welcome to evaluate and possibly remedy one or more findbugs errors. For precisely that reason I put comments in the exclusion file, so one can see which errors have been simply hidden and which have been truely evaluated. Simon Thanks, Vincent On Mon, Feb 21, 2011 at 06:15:13PM +, Vincent Hennebert wrote: Hi, If we solve issues raised by FindBugs by listing them in an ignore file, is there still a point to use FindBugs at all? It seems to me that some of those issues deserve to be fixed. They seem to point out genuine problems in the code. Vincent On 18/02/11 08:18, spepp...@apache.org wrote: Author: spepping Date: Fri Feb 18 08:18:04 2011 New Revision: 1071912 URL: http://svn.apache.org/viewvc?rev=1071912view=rev Log: Fixing checkstyle errors and hiding fingbugs errors Modified: xmlgraphics/fop/trunk/findbugs-exclude.xml xmlgraphics/fop/trunk/src/java/org/apache/fop/render/pdf/PDFImageHandlerSVG.java Modified: xmlgraphics/fop/trunk/findbugs-exclude.xml URL: http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/findbugs-exclude.xml?rev=1071912r1=1071911r2=1071912view=diff
junit tests in nightly builds [was: Re: Solving FindBugs issue]
On Tue, Feb 22, 2011 at 11:25:20AM -0700, Glenn Adams wrote: I notice also that the nightly build target does not run all the junit tests. It would be better if it run all of them plus checkstyle and findbugs. Many junit tests require a display. Nightly builds are run in a headless configuration, hence I had to disable many junit tests. At nightly builds there is no one to check checkstyle and findbugs errors and warnings; therefore there is no point in running them. Simon
