I have tried to solve the problem of the last line in a justified block, which sometimes is justified too, instead of being left-aligned. This problem arises when the fo:block ends with spaces (the last TextLM's text ends with spaces), or with a LF followed by spaces (the last TLM's text contains only spaces), while if the block ends with a non-whitespace character it is handled right; the test fo file I'm going to attach shows these different situations. So, a possible solution could be avoiding these problematic situations to happen: as the end of a block always implies a line break (or not?) the ending spaces can be removed. This is what the proposed patch I'm going to attach does, slightly modifying the Block.handleWhiteSpace() method: - define another RecursiveCharIterator, called spaceIter, initialized null - inside the while loop, it is updated (if it's null) cloning charIter before nextChar() is called, and set to null again if the current character is not a space; so, if it is not null it points to the first element in a sequence of spaces - once the loop ends, if spaceIter is not null it is used to remove thr spaces at the end of the block Without any other code change, this seem to solve the problem. An alternative approach: why doesn't the LineLM change text alignment for the last line? Alignment is changed if (bTextAlignment == TextAlign.JUSTIFY && (prevBP.isForcedBreak() || isFinished())) { and in the problematic cases isFinished() returns "false", so text alignment is not changed. This is what happens during the LineLayoutManager.getNextBreakPoss(): BreakPoss a || BreakPoss b vv |.... end of text. | - inside the while loop, bp = curLM.getNextBreakPoss() returns the break possibility "a", which could be used to end the line, and is saved in prevBP - at the next iteration inside the while loop, bp = curLM.getNextBreakPoss() returns the break possibility "b", which is not good (bBreakOK == false) - the TLM has finished, so at the next iteration getChildLM() returns null, and the while loop ends - the LLM calls setFinished(), because there are no more child LM - now prevBP=a, bp=b, so prevBP != bp - oddly, prevCouldEndLine(prevBP) is false - so the reset() function is called, which calls AbstractLayoutManager.reset (Position), which has these lines: if (isFinished()) { setFinished(false); } (in the other problematic situation it's a different TLM which returns "b", but the LLM behaviour is quite the same) It seems to me that the value returned by prevCouldEndLine(prevBP) is wrong, but I didn't investigate any further as I was quite satisfied with the result obtained in the other way. Luca
Created attachment 11199 [details] test fo file
Created attachment 11200 [details] proposed patch
Hi Luca, I'm not certain that your first solution of changing Block.handleWhiteSpace() to ignore spaces in last TextLM text is bullet proof. I havent done any testing, so it may be okay. Certainly for ordinary Blocks it may be fine, but for other FOs, which inherit from Block, this may not be acceptable. For example, table-cells, with auto-layout feature on, spaces at the end might be relevant. I'm not rejecting your solution, but I would like to explore your second idea a bit further. Another reason why it may not be good to change the logic in Block.handleWhiteSpace() is that it currently follows the description for whitespace handling in the spec fairly closely and putting other conditions in, will detract from this.
Hi I have tried to follow the other idea, i.e. understand why reset() is called in these problematic situations, and maybe I have found an alternative patch which doesn't involve white-space-treatment changes. Problems in the justification of the last line arise when the TextLayoutManagers returns to the LineLM, in sequence: - a valid BreakPoss, stored in the ArrayList vecInlineBreaks and in the variable prevBP - one or more invalid BreakPoss, for which isSuppressible() is true, stored only in vecInlineBreaks - nothing else, as ther are no more child LM with something to layout An important detail: if the spaces are at the end of the text of a TLM, prevBP has the REST_ARE_SUPPRESS_AT_LB flag set; in the other case it hasn't. The invalid bp have the ALL_ARE_SUPPRESS_AT_LB flag set. The method reset() is called if bp != prevBP (the last returned bp is invalid) && !prevCouldEndLine(prevBP) (we cannot ignore what is after prevBP) and I think in our problematic situations !prevCouldEndLine(prevBP) should be false, i.e. prevCouldEndLine(prevBP) should be true. So, this is how I would calculate prevCouldEndLine(prev): 1) first of all, check if isFinished() is true: if it's not, there are other childLM and we cannot ignore any space 2) starting from the last element in vecInlineBreaks and moving backward (stopping when prev is reached), check if bp are suppressible At the moment, prevCouldEndLine calls bp.couldEndLine(), which checks the REST_ARE_SUPPRESS_AT_LB bit, which can be true only for a valid BreakPoss followed by suppressible spaces, and not for the invalid BreakPoss; with my patch, this method would be very similar to allAreSuppressible. There is only another change to do in the getNextBreakPoss method: if (bpDim.min > availIPD.max) and !(bTextAlignment == TextAlign.JUSTIFY || prevBP == null) the bp should be added to vecInlineBreaks whether prevBP is null or not, otherwise if the last line of a non-justified block contains only a word it is not shown. You can see this using the sample fo file I attached before: the first block is left-aligned, and its third and last line is "spaces."; without this last change, the line disappears. In this case reset() should have been called, but prevBP is the last BreakPoss in vecInlineBreaks, so prevCouldEndLine(prevBP) returns true. I'm going to attach this alternative proposed patch. Luca
Created attachment 11317 [details] Alternative patch
Alternative patch Applied, thanks very much
Luca, This is a good patch, and Chris has already applied it. I have a few remarks. 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 11386 [details] The patch as described
Simon, I have moved your patch to a new bug report because this one has been closed. Also, it quickly becomes confusing as to what is going on when there are several patches attached to one bug report. http://issues.apache.org/bugzilla/show_bug.cgi?id=28706
batch transition pre-FOP1.0 resolved+fixed bugs to closed+fixed