<Swing Dev> [13] RFR JDK-8214702:Wrong text position for whitespaced string in printing Swing text
Prasanta Sadhukhan
prasanta.sadhukhan at oracle.com
Fri May 17 05:05:45 UTC 2019
Hi Sergey,
On 16-May-19 9:13 PM, Sergey Bylokhov wrote:
> Hi, Prasanta.
> Should not we use the real "screen" FontRenderContext instead of
> "DEFAULT_FRC"?
Yes, I guess it was mentioned in one of the review comment earliter,
that it may not be the cause of this problem, but it needs to be changed.
> Did you check do we need to update the similat cases in the same
> class(we use getJustifiedLayout() 4 times mostly in the same code
> pattern)?
Might as well. I updated 3 of them and the 4th one is with
AttributeCharaterIterator (and not dealing with String as such unlike
other 3) so did not update.
Modified webrev with the change
http://cr.openjdk.java.net/~psadhukhan/8214702/webrev.5/
Regards
Prasanta
>
> On 15/05/2019 22:23, Prasanta Sadhukhan wrote:
>> Not sure if you are saying it's ok to "push"? I guess I need a 2nd
>> reviewer if you are ok with this.
>>
>> Regards
>> Prasanta
>> On 16-May-19 12:01 AM, Phil Race wrote:
>>> OK. Let's try that. I am sure it is still going to be non-ideal in a
>>> couple of ways
>>> 1) where printed is shorter than screenwidth and we don't adjust,
>>> there may
>>> be space after the text and before the "edge" of the component, but
>>> at least
>>> there's no clipping and the text should look natural initself
>>>
>>> 2) Where printed is longer than screenwidth and we do adjust we may
>>> still
>>> run into similar cases as this ..
>>>
>>> -phil.
>>>
>>> On 4/19/19 12:56 AM, Prasanta Sadhukhan wrote:
>>>>
>>>>
>>>>
>>>> On 18-Apr-19 12:31 AM, Phil Race wrote:
>>>>>
>>>>>
>>>>> On 4/16/19 3:38 AM, Prasanta Sadhukhan wrote:
>>>>>>
>>>>>> 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.
>>>>>
>>>>> The spaces being used to add needed or absorb surplus space is
>>>>> what I understood the current code to be doing,
>>>>>
>>>>> However I am not sure what you mean by this :-
>>>>> "but handle everything else by modifying position before and after,"
>>>>>
>>>>> Code run through text layout will always have glyph positions
>>>>> assigned, so I would suppose modifying the position
>>>>> is how the spaces were handled too .. and of course this means
>>>>> running through all preceding glyphs to make
>>>>> it consistent .. and I thought it was only adjusting the spaces,
>>>>> so what is "everything else"
>>>> It seems when TextLineComponent.applyJustificationDeltas() is
>>>> called, ExtendedTextSourceLabel.applyJustificationDeltas() handles
>>>> that
>>>> http://hg.openjdk.java.net/jdk/client/file/dc6c5c53669b/src/java.desktop/share/classes/sun/font/ExtendedTextSourceLabel.java#l1015
>>>>
>>>> where it handles "whitespace" a bit different (if block) from other
>>>> glyph (else block) where advance is is not considered, which is
>>>> also conveyed in the comment @1011
>>>>>>
>>>>>> Since it was mentioned in TextLayout.getJustifiedLayout() that
>>>>>> " For best results, justificationWidth should not be too
>>>>>> different from the current advance of the line"
>>>>>
>>>>> So for "best" results don't try to adjust a string that's 3 words
>>>>> and 80 pixels to a 500 pixel width.
>>>>>
>>>>>> 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.
>>>>>>
>>>>>
>>>>> I had proposed investigating what happens if you simply don't use
>>>>> justification when the text will fit.
>>>>> Maybe you are refining that but I am not sure about the details.
>>>>> 10 is one arbitrary number and is not proportional to the length -
>>>>> which is what I would be
>>>>> thinking about first in succh an approach
>>>>> And I am not even sure you mean what you say.
>>>>> You say "more" than 10, yet your code implements "less" than 10.
>>>> It was a typo in mail.
>>>> Modified webrev to not use justification if string width of text
>>>> is within screenWidth
>>>> http://cr.openjdk.java.net/~psadhukhan/8214702/webrev.4/
>>>>
>>>> Regards
>>>> Prasanta
>>>>> And moreover its an absolute value you are using, which
>>>>> re-introduces the clipping problem, doesn't it ?
>>>>> ie you are purely looking at the difference and it isn't what I
>>>>> has proposed and I thought you were trying.
>>>>>
>>>>> -phil
>>>>>
>>>>>> 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);
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
>
More information about the swing-dev
mailing list