Simon Pepping comments copied from bug 28314: 1. The patch introduces a trailing space in some cases. This can be seen in the area tree dump, in the second block in your test FO file. This problem can be remedied by adding an else branch, which removes all BPs after prevBP from vecInlineBreaks if we do not reset. 2. The disappearance of the word 'spaces' is due to the fact that in this case bp has not yet been added to vecInlineBreaks, so that prevCouldEndLine is applied on prevBP only, in which case it returns true immediately. We want it to return false in this case so that reset is called and bp is not silently dropped. The problem can be remedied by testing against this condition before checking for suppressibility. I prefer that above your adding bp to vecInlineBreaks, of which the side effects in other cases are unknown. prevCouldEndLine could then be written as: private boolean prevCouldEndLine(BreakPoss prev) { if (!isFinished()) { return false; } if (vecInlineBreaks.get(vecInlineBreaks.size() - 1) == prev) { return false; } return allAreSuppressible(prev); } My attached patch does the above. It also does away with prevCouldEndLine by inserting all the conditions directly in the code and reusing allAreSuppressible. Simon
Created attachment 11392 [details] Simon's patch
Hi Simon, I can see the trailing space in the second paragraph as you describe. However, both occurences of the word 'spaces' exists both in PDF output and the Area Tree. I am wondering if adding the else branch as you suggested to fix (1) actually causes issue (2). If this is the case, I may be inclined to reject this patch, because I'm not sure if (1) is genuinely a bug or not. I may be wrong here, as I am not up on all the details of the spec, but if you add spaces at the end of some text which is then dealt with by whitespace handling rules, in this case, multiple spaces collapsed into 1, then wouldnt you expect these to appear in the output? Perhaps the real bug here is the spaces collapsed in the third paragraph do not generate a single trailing space? Chris
Chris, A trailing space at the end of the last line of a paragraph is certainly a problem; it simply should not be there. While it does not cause much harm, it is better to avoid it. It does point to a slight problem in the logic of the code. The addition of the else branch remedies that problem. The word 'spaces' is there allright without the patch. The addition of the else branch has nothing to do with it. The argument refers back to patch 28314, where the word 'spaces' disappeared unless a change was applied which added bp to vecInlineBreaks. I argue that I am not completely happy with this change, and prefer to review the return value of prevCouldEndLine, such that it returns false if prev is the last item in vecInlineBreaks: if (vecInlineBreaks.get(vecInlineBreaks.size() - 1) == prev) { return false; } With this addition to prevCouldEndLine, instead of adding bp to vecInlineBreaks, the disappearance of the word 'spaces' is avoided as well. Simon
Hi Simon, thanks for your additional comments. On your first point regarding trailing spaces, just to be clear, are you saying there shouldnt be trailing spaces according to the XSL-FO specification, or is this your opinion? Either way, you may be right, I just want to understand this problem from a specification point of view. After all there is a lot of talk about whitespace handling in the spec, and my understanding was that this is all dealt with in Block.handleWhiteSpace and so any whitespace left after that is intentional by the user, and by preserving it FOP is conforming with the spec. On your second point, I'm afraid I still dont follow completely. You are analysing the problem from a code perspective and seeing a possible flaw in the logic. I understand that, and your reasoning looks good. However, I am taking a step back and saying I do not see the word "spaces" disappearing. Dont forget that patch 28314 is applied to CVS, so if that patch is the cause of the word disappearing I'm a little puzzled why it doesnt disappear when I run the test. If you have a different test case, then please attach it. Is anyone else observing this anomaly? Chris
1. See XSL-PR/slice7.html#suppress-at-line-break, whose default value is 'auto'. Block.handleWhiteSpace cannot handle this because it runs at the refinement stage. 2. This point is about code level. When you take a step back, you cannot see the point.
Thanks for the link Simon. Although this patch may be redundant now with Luca's proposal on the way.
Applied the patch in a rewritten form.
batch transition pre-FOP1.0 resolved+fixed bugs to closed+fixed