<Swing Dev> [13] RFR JDK-8214702:Wrong text position for whitespaced string in printing Swing text
Prasanta Sadhukhan
prasanta.sadhukhan at oracle.com
Tue May 21 08:49:18 UTC 2019
Thanks for your review. I have run the relevant jck/regression test and
I do not observe any new issues. As for long lines, I will split it
while pushing.
Regards
Prasanta
On 20-May-19 11:39 PM, Sergey Bylokhov wrote:
> Hi, Prasanta.
>
> This version looks fine, but I have two comments:
> - Please split the long lines in the fix.
> - Can you please confirm that the tests(jck/regression) are passed on
> the latest version, and there are no new issues.
>
> On 16/05/2019 22:05, Prasanta Sadhukhan wrote:
>> 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