<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