<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