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);
