<Swing Dev> [13] RFR JDK-8214702:Wrong text position for whitespaced string in printing Swing text
Prasanta Sadhukhan
prasanta.sadhukhan at oracle.com
Fri Apr 19 07:56:09 UTC 2019
On 18-Apr-19 12:31 AM, Phil Race wrote:
>
>
> On 4/16/19 3:38 AM, Prasanta Sadhukhan wrote:
>>
>> Hi Phil,
>>
>> It seems screenWidth or "advance of the string at screen-resolution"
>> is 533 whereas string advance calculated using printer fontmetrics is
>> 503
>>
>> Now, TextLine#getJustifiedLine() [called from
>> TextLayout.getJustifiedLayout] again calculates "justifyAdvance" for
>> TextLineComponent which comes out to be 503,then it calculates
>> the actual justification "delta" by subtracting justifyAdvance from
>> screenWidth which is 533-503=30 and it then does
>> TextJustifier.justify(delta)
>> which calculates the amount by which each side of each glyph should
>> grow or shrink.
>>
>> Then TextLine.getJustifiedLine() applies this value by calling
>> TextLineComponent.applyJustificationDeltas() where it
>> handle whitespace by modifying advance but handle everything else
>> by modifying position before and after,
>> so "spaces" seem to grow in size resulting in shifting of text with
>> whitespaces.
>
> The spaces being used to add needed or absorb surplus space is what I
> understood the current code to be doing,
>
> However I am not sure what you mean by this :-
> "but handle everything else by modifying position before and after,"
>
> Code run through text layout will always have glyph positions
> assigned, so I would suppose modifying the position
> is how the spaces were handled too .. and of course this means running
> through all preceding glyphs to make
> it consistent .. and I thought it was only adjusting the spaces, so
> what is "everything else"
It seems when TextLineComponent.applyJustificationDeltas() is called,
ExtendedTextSourceLabel.applyJustificationDeltas() handles that
http://hg.openjdk.java.net/jdk/client/file/dc6c5c53669b/src/java.desktop/share/classes/sun/font/ExtendedTextSourceLabel.java#l1015
where it handles "whitespace" a bit different (if block) from other
glyph (else block) where advance is is not considered, which is also
conveyed in the comment @1011
>>
>> Since it was mentioned in TextLayout.getJustifiedLayout() that "
>> For best results, justificationWidth should not be too different from
>> the current advance of the line"
>
> So for "best" results don't try to adjust a string that's 3 words and
> 80 pixels to a 500 pixel width.
>
>> I am proposing a fix to conditionally call
>> TextLayout.getJustifiedLayout() only if the difference between
>> justificationWidth and advance with for printer is more than 10.
>>
>
> I had proposed investigating what happens if you simply don't use
> justification when the text will fit.
> Maybe you are refining that but I am not sure about the details.
> 10 is one arbitrary number and is not proportional to the length -
> which is what I would be
> thinking about first in succh an approach
> And I am not even sure you mean what you say.
> You say "more" than 10, yet your code implements "less" than 10.
It was a typo in mail.
Modified webrev to not use justification if string width of text is
within screenWidth
http://cr.openjdk.java.net/~psadhukhan/8214702/webrev.4/
Regards
Prasanta
> And moreover its an absolute value you are using, which re-introduces
> the clipping problem, doesn't it ?
> ie you are purely looking at the difference and it isn't what I has
> proposed and I thought you were trying.
>
> -phil
>
>> webrev: http://cr.openjdk.java.net/~psadhukhan/8214702/webrev.3/
>>
>> Regards
>> Prasanta
>> On 26-Feb-19 12:30 PM, Philip Race wrote:
>>>
>>>
>>> On 2/25/19, 10:21 PM, Prasanta Sadhukhan wrote:
>>>>
>>>> Thanks Phil for review. So, you are doubting it will regress swing
>>>> printing tests. As you told earlier, I have ran the following
>>>> regression test with this fix
>>>>
>>>
>>> It may not regress a test if the test is not being tested under the
>>> same conditions for which it is created but I am telling you for a
>>> fact that the fix is wrong. screenWidth should be "width on the screen"
>>>>
>>>> (https://bugs.openjdk.java.net/browse/JDK-6488219 The bug above is
>>>> covered by java/awt/print/PrinterJob/SwingUIText.java)
>>>> and I did not notice any regression. Any other test we have for
>>>> swing printing that we can run?
>>>>
>>>
>>> No idea which tests will show this today but I know it is an issue.
>>> No way we can push this and then just wait for the complaints.
>>>
>>>> >>Previously we were using DEFAULT_FRC to make it a screen width
>>>> which except for maybe needing to be updated for hi-dpi screens is
>>>> what we want.
>>>> This issue was there from jdk1.6. If it is for hidpi screen, we
>>>> would have seen it from jdk9 onwards where we supported hidpi, no?
>>>
>>> What I am saying here is that DEFAULT_FRC means "screen frc" and
>>> I think that should have been updated in 1.9 but was missed because
>>> it (hidpi for windows) was not a small or contained task.
>>> This is an ancilliary observation of something that should be looked
>>> at entirely independent of this bug.
>>>
>>> -phil.
>>>
>>>>
>>>> Regards
>>>> Prasanta
>>>> On 26-Feb-19 3:25 AM, Phil Race wrote:
>>>>> The current fix confused me for a while as I could not see how it was
>>>>> at all different than the existing code, since I can't imagine
>>>>> when we'd
>>>>> ever take the "else" branch here :
>>>>> 533 TextLayout layout;
>>>>> 534 if (!isFontRenderContextPrintCompatible(frc, deviceFRC)) {
>>>>> 535 layout = createTextLayout(c, text, g2d.getFont(), deviceFRC);
>>>>> 536 } else {
>>>>> 537 layout = createTextLayout(c, text, g2d.getFont(), frc);
>>>>> 538 } Eventually when walking through it I noticed this 531
>>>>> FontMetrics fm = g2d.getFontMetrics();
>>>>> 532 float screenWidth = SwingUtilities2.stringWidth(c, fm
>>>>> ,trimmedText);
>>>>>
>>>>> "fm" from line 532 is getting a FontMetrics from the PRINTER - ie
>>>>> the scaled FontRenderContext.
>>>>> It then uses this to calculate the advance width for such a case -
>>>>> ie the printer
>>>>> but then *assigns it to a variable called screenWidth*.
>>>>>
>>>>> Previously we were using DEFAULT_FRC to make it a screen width
>>>>> which except
>>>>> for maybe needing to be updated for hi-dpi screens is what we want.
>>>>>
>>>>> So in the updated proposed fix the wrong width is passed to
>>>>> getJustifiedLayout().
>>>>>
>>>>> This may not matter here because there is plenty of space, but in
>>>>> other cases
>>>>> Swing printing will be clipped as a result. And there were many,
>>>>> many, bug reports about
>>>>> that. Which is why the code is laying out to the screenwidth
>>>>> because that is where the
>>>>> UI component size available came from. Buttons & Label text are
>>>>> the typical cases where
>>>>> this showed up.
>>>>>
>>>>> There maybe other things to change, as well but the incorrect
>>>>> screenWidth is the
>>>>> main problem I see here.
>>>>>
>>>>> -phil.
>>>>>
>>>>> On 2/25/19 12:05 AM, Prasanta Sadhukhan wrote:
>>>>>>
>>>>>>
>>>>>> On 21-Feb-19 4:50 AM, Sergey Bylokhov wrote:
>>>>>>> On 13/02/2019 22:53, Prasanta Sadhukhan wrote:
>>>>>>>> Hi Sergey,
>>>>>>>>
>>>>>>>> I believe drawChars() also has same printing issue [and should
>>>>>>>> be changed like modified drawString()] but I am not able to
>>>>>>>> test it as reproducer testcase uses JLabel whose constructor
>>>>>>>> can only accept "String" and not char[] so I can only test
>>>>>>>> drawString(). Using drawChars() implementation in drawString()
>>>>>>>> still reproduces the issue.
>>>>>>>
>>>>>>> Is it possible temporary replace the call to drawString() by the
>>>>>>> drawChars(), to check how drawChars() will work?
>>>>>> As I told, it behaves similarly to unmodified drawString and the
>>>>>> issue can still be seen. I think we should commit this
>>>>>> drawString() change in this fix and I can open another bug to
>>>>>> investigate drawChars() impl and reproducer. Will that be fine?
>>>>>>
>>>>>> Regards
>>>>>> Prasanta
>>>>>>> or probably we can implement drawChars() on top of drawString()?
>>>>>>>
>>>>>>>>
>>>>>>>> Regards
>>>>>>>> Prasanta
>>>>>>>> On 14-Feb-19 4:12 AM, Sergey Bylokhov wrote:
>>>>>>>>> Hi, Prasanta.
>>>>>>>>>
>>>>>>>>>> I modified the fix to use deviceFRC if not compatible and in
>>>>>>>>>> sync with the comment which says "obtain a TextLayout with
>>>>>>>>>> advances for the printer graphics FRC"
>>>>>>>>>> I used SwingUtilies2.getStringWidth() which calculates the
>>>>>>>>>> advances of the string if text layouting is used.
>>>>>>>>>>
>>>>>>>>>> http://cr.openjdk.java.net/~psadhukhan/8214702/webrev.2/
>>>>>>>>>
>>>>>>>>> Can you please take a look to the existed drawChars() method,
>>>>>>>>> which is implemented in the similar way as drawStringImpl()
>>>>>>>>> and new version of drawString(), but they have some small
>>>>>>>>> difference. Why we cannot use the same logic?
>>>>>>>>>
>>>>>>>>> For example in the drawChars:
>>>>>>>>> =========
>>>>>>>>> FontRenderContext deviceFontRenderContext = g2d.
>>>>>>>>> getFontRenderContext();
>>>>>>>>> FontRenderContext frc = getFontRenderContext(c);
>>>>>>>>> if (frc != null &&
>>>>>>>>> !isFontRenderContextPrintCompatible
>>>>>>>>> (deviceFontRenderContext, frc)) {
>>>>>>>>> String text = new String(data, offset, length);
>>>>>>>>> TextLayout layout = new TextLayout(text,
>>>>>>>>> g2d.getFont(),
>>>>>>>>> deviceFontRenderContext);
>>>>>>>>> String trimmedText = trimTrailingSpaces(text);
>>>>>>>>> if (!trimmedText.isEmpty()) {
>>>>>>>>> float screenWidth = (float)g2d.getFont().
>>>>>>>>> getStringBounds(trimmedText, frc).getWidth();
>>>>>>>>> layout =
>>>>>>>>> layout.getJustifiedLayout(screenWidth);
>>>>>>>>>
>>>>>>>>> ==========
>>>>>>>>> Similar but not the same logic in the fix:
>>>>>>>>>
>>>>>>>>> 524 FontRenderContext frc =
>>>>>>>>> getFontRenderContext(c);
>>>>>>>>> 525 if (frc.isAntiAliased() ||
>>>>>>>>> frc.usesFractionalMetrics()) {
>>>>>>>>> 526 frc = new
>>>>>>>>> FontRenderContext(frc.getTransform(), false, false);
>>>>>>>>> 527 }
>>>>>>>>> 528 FontRenderContext deviceFRC =
>>>>>>>>> g2d.getFontRenderContext();
>>>>>>>>> 529 String trimmedText =
>>>>>>>>> trimTrailingSpaces(text);
>>>>>>>>> 530 if (!trimmedText.isEmpty()) {
>>>>>>>>> 531 FontMetrics fm = g2d.getFontMetrics();
>>>>>>>>> 532 float screenWidth =
>>>>>>>>> SwingUtilities2.stringWidth(c, fm ,trimmedText);
>>>>>>>>> 533 TextLayout layout;
>>>>>>>>> 534 if
>>>>>>>>> (!isFontRenderContextPrintCompatible(frc, deviceFRC)) {
>>>>>>>>> 535 layout = createTextLayout(c,
>>>>>>>>> text, g2d.getFont(), deviceFRC);
>>>>>>>>> 536 } else {
>>>>>>>>> 537 layout = createTextLayout(c,
>>>>>>>>> text, g2d.getFont(), frc);
>>>>>>>>> 538 }
>>>>>>>>> 540 layout =
>>>>>>>>> layout.getJustifiedLayout(screenWidth);
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/swing-dev/attachments/20190419/06ab8521/attachment-0001.html>
More information about the swing-dev
mailing list