<Swing Dev> [13] RFR JDK-8214702:Wrong text position for whitespaced string in printing Swing text
Prasanta Sadhukhan
prasanta.sadhukhan at oracle.com
Thu May 16 05:23:35 UTC 2019
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);
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/swing-dev/attachments/20190516/a0ddd5dd/attachment-0001.html>
More information about the swing-dev
mailing list