<Swing Dev> [13] RFR JDK-8214702:Wrong text position for whitespaced string in printing Swing text

Philip Race philip.race at oracle.com
Tue Feb 26 07:00:50 UTC 2019



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/20190225/bfa6af21/attachment-0001.html>


More information about the swing-dev mailing list