<Swing Dev> [13] RFR JDK-8214702:Wrong text position for whitespaced string in printing Swing text
Prasanta Sadhukhan
prasanta.sadhukhan at oracle.com
Tue Apr 16 10:38:22 UTC 2019
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.
Since it was mentioned in TextLayout.getJustifiedLayout() that " For
best results, justificationWidth should not be too different from the
current advance of the line"
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.
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/20190416/95b5e6f6/attachment-0001.html>
More information about the swing-dev
mailing list