<Swing Dev> [13] RFR JDK-8214702:Wrong text position for whitespaced string in printing Swing text
Prasanta Sadhukhan
prasanta.sadhukhan at oracle.com
Wed Apr 3 06:56:46 UTC 2019
Hi Phil,
Will you please be able to provide me the changeset for JDK-6488219 and
JDK-6760148 as I am not able to see this pre-mercurial fixes, which will
help me to narrow down to probable reason?
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/20190403/78159e0e/attachment-0001.html>
More information about the swing-dev
mailing list