<Swing Dev> [13] RFR JDK-8214702:Wrong text position for whitespaced string in printing Swing text
Philip Race
philip.race at oracle.com
Thu Apr 4 03:15:49 UTC 2019
I am not sure how I can.
- it pre-dates mercurial so there isn't likely to be anything as neat as
a changeset.
SCCS was file based, so you can't see a complete change very easily
without the
right tools which won't run on anything you have ...
- it pre-dates open source so there isn't anything like that I can
publish even if
it existed.
- I am not sure it will actually help solve this problem anyway.
-phil.
On 4/2/19, 11:56 PM, Prasanta Sadhukhan wrote:
>
> 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/a5325264/attachment-0001.html>
More information about the swing-dev
mailing list