<Swing Dev> RFR: 8160720: [TEST_BUG] javax/swing/SwingUtilities/TestBadBreak/TestBadBreak.java

classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

<Swing Dev> RFR: 8160720: [TEST_BUG] javax/swing/SwingUtilities/TestBadBreak/TestBadBreak.java

Prasanta Sadhukhan-2
Test hardcodes bufferedimage and frame bounds so it does not capture correct size if scaling factor is used.
The solution is to always use the same uiscale=1, an updated test still can reproduce the JDK-8015085 issue without fix. Additionally, moved the frame to centre of screen for better mach5 stability.
Mach5 job running for several iterations on all platforms is ok. Link in JBS.

-------------

Commit messages:
 - 8160720: [TEST_BUG] javax/swing/SwingUtilities/TestBadBreak/TestBadBreak.java

Changes: https://git.openjdk.java.net/jdk/pull/2502/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2502&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8160720
  Stats: 6 lines in 2 files changed: 4 ins; 1 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2502.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2502/head:pull/2502

PR: https://git.openjdk.java.net/jdk/pull/2502
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8160720: [TEST_BUG] javax/swing/SwingUtilities/TestBadBreak/TestBadBreak.java

Sergey Bylokhov-2
On Wed, 10 Feb 2021 10:33:03 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

> Test hardcodes bufferedimage and frame bounds so it does not capture correct size if scaling factor is used.
> The solution is to always use the same uiscale=1, an updated test still can reproduce the JDK-8015085 issue without fix. Additionally, moved the frame to centre of screen for better mach5 stability.
> Mach5 job running for several iterations on all platforms is ok. Link in JBS.

test/jdk/javax/swing/SwingUtilities/TestBadBreak/TestBadBreak.java line 94:

> 92:         SwingUtilities.invokeAndWait(() -> frame.setVisible(true));
> 93:         robot.waitForIdle();
> 94:         robot.delay(1000);

Why do you need this delay and then one more in between the "test"?

test/jdk/javax/swing/SwingUtilities/TestBadBreak/TestBadBreak.java line 43:

> 41:  * @summary Shortening via " ... " is broken for Strings containing a combining
> 42:  *          diaeresis.
> 43:  * @run main/othervm -Dsun.java2d.uiScale=1 TestBadBreak

I missed the point why the hardcoded values do not work in this test, both frame and robot should take the coordinates in the user's space.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2502
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8160720: [TEST_BUG] javax/swing/SwingUtilities/TestBadBreak/TestBadBreak.java

Prasanta Sadhukhan-2
On Wed, 10 Feb 2021 11:18:38 GMT, Sergey Bylokhov <[hidden email]> wrote:

>> Test hardcodes bufferedimage and frame bounds so it does not capture correct size if scaling factor is used.
>> The solution is to always use the same uiscale=1, an updated test still can reproduce the JDK-8015085 issue without fix. Additionally, moved the frame to centre of screen for better mach5 stability.
>> Mach5 job running for several iterations on all platforms is ok. Link in JBS.
>
> test/jdk/javax/swing/SwingUtilities/TestBadBreak/TestBadBreak.java line 94:
>
>> 92:         SwingUtilities.invokeAndWait(() -> frame.setVisible(true));
>> 93:         robot.waitForIdle();
>> 94:         robot.delay(1000);
>
> Why do you need this delay and then one more in between the "test"?

This is between visible and destroy..The other one before start of the 2nd test which will crate 2nd frame..

> test/jdk/javax/swing/SwingUtilities/TestBadBreak/TestBadBreak.java line 43:
>
>> 41:  * @summary Shortening via " ... " is broken for Strings containing a combining
>> 42:  *          diaeresis.
>> 43:  * @run main/othervm -Dsun.java2d.uiScale=1 TestBadBreak
>
> I missed the point why the hardcoded values do not work in this test, both frame and robot should take the coordinates in the user's space.

It uses hardcoded 200,90 for frame.setBounds.I guess it should be multiplied by scale factor for correct bounds to be used.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2502
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8160720: [TEST_BUG] javax/swing/SwingUtilities/TestBadBreak/TestBadBreak.java

Sergey Bylokhov-2
On Wed, 10 Feb 2021 11:45:35 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

>> test/jdk/javax/swing/SwingUtilities/TestBadBreak/TestBadBreak.java line 43:
>>
>>> 41:  * @summary Shortening via " ... " is broken for Strings containing a combining
>>> 42:  *          diaeresis.
>>> 43:  * @run main/othervm -Dsun.java2d.uiScale=1 TestBadBreak
>>
>> I missed the point why the hardcoded values do not work in this test, both frame and robot should take the coordinates in the user's space.
>
> It uses hardcoded 200,90 for frame.setBounds.I guess it should be multiplied by scale factor for correct bounds to be used.

Both coordinates robot/frame use the user's space and internally should be transformed to the device space. But that transformation should be the same, there are should not be differences in the final result for both.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2502
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8160720: [TEST_BUG] javax/swing/SwingUtilities/TestBadBreak/TestBadBreak.java

Sergey Bylokhov-2
In reply to this post by Prasanta Sadhukhan-2
On Wed, 10 Feb 2021 11:43:11 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

>> test/jdk/javax/swing/SwingUtilities/TestBadBreak/TestBadBreak.java line 94:
>>
>>> 92:         SwingUtilities.invokeAndWait(() -> frame.setVisible(true));
>>> 93:         robot.waitForIdle();
>>> 94:         robot.delay(1000);
>>
>> Why do you need this delay and then one more in between the "test"?
>
> This is between visible and destroy..The other one before start of the 2nd test which will crate 2nd frame..

And why do you need both? The test does not use the robot, so the test can show all windows at once.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2502
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8160720: [TEST_BUG] javax/swing/SwingUtilities/TestBadBreak/TestBadBreak.java

Sergey Bylokhov-2
On Wed, 10 Feb 2021 21:34:21 GMT, Sergey Bylokhov <[hidden email]> wrote:

>> This is between visible and destroy..The other one before start of the 2nd test which will crate 2nd frame..
>
> And why do you need both? The test does not use the robot, so the test can show all windows at once.

BTW since the robot is not used it is unclear how the setUndecorated:true/false and setLocationRelativeTo() may affect the test.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2502
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8160720: [TEST_BUG] javax/swing/SwingUtilities/TestBadBreak/TestBadBreak.java

Sergey Bylokhov-2
In reply to this post by Sergey Bylokhov-2
On Wed, 10 Feb 2021 21:31:21 GMT, Sergey Bylokhov <[hidden email]> wrote:

>> It uses hardcoded 200,90 for frame.setBounds.I guess it should be multiplied by scale factor for correct bounds to be used.
>
> Both coordinates robot/frame use the user's space and internally should be transformed to the device space. But that transformation should be the same, there are should not be differences in the final result for both.

BTW the test itself draws the button to the BufferedImage so uiscale should not affect that rendering, both subtests should work similarly.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2502
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8160720: [TEST_BUG] javax/swing/SwingUtilities/TestBadBreak/TestBadBreak.java

Alexey Ivanov-2
In reply to this post by Sergey Bylokhov-2
On Wed, 10 Feb 2021 21:37:35 GMT, Sergey Bylokhov <[hidden email]> wrote:

>> And why do you need both? The test does not use the robot, so the test can show all windows at once.
>
> BTW since the robot is not used it is unclear how the setUndecorated:true/false and setLocationRelativeTo() may affect the test.

Since the text is rendered in JLabel and it's rendered into a BufferedImage, do we need the frame at all?
If no frame is shown, the test can be headless, can't it?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2502
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8160720: [TEST_BUG] javax/swing/SwingUtilities/TestBadBreak/TestBadBreak.java

Prasanta Sadhukhan-2
In reply to this post by Sergey Bylokhov-2
On Wed, 10 Feb 2021 21:49:40 GMT, Sergey Bylokhov <[hidden email]> wrote:

>> Both coordinates robot/frame use the user's space and internally should be transformed to the device space. But that transformation should be the same, there are should not be differences in the final result for both.
>
> BTW the test itself draws the button to the BufferedImage so uiscale should not affect that rendering, both subtests should work similarly.

I believe similar thing was done for https://github.com/openjdk/jdk/pull/950/files where the panel was drawn to bufferedimage but uiScale was set to 1 to make the test stable and robot was also used there to just delay but setUndecorated and setLocationRelativeTo() was used.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2502
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8160720: [TEST_BUG] javax/swing/SwingUtilities/TestBadBreak/TestBadBreak.java

Sergey Bylokhov-2
On Thu, 11 Feb 2021 04:44:56 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

>> BTW the test itself draws the button to the BufferedImage so uiscale should not affect that rendering, both subtests should work similarly.
>
> I believe similar thing was done for https://github.com/openjdk/jdk/pull/950/files where the panel was drawn to bufferedimage but uiScale was set to 1 to make the test stable and robot was also used there to just delay but setUndecorated and setLocationRelativeTo() was used.

That test uses a robot, and it may fail if the windows appeared under dock/etc.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2502
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8160720: [TEST_BUG] javax/swing/SwingUtilities/TestBadBreak/TestBadBreak.java

Sergey Bylokhov-2
In reply to this post by Alexey Ivanov-2
On Wed, 10 Feb 2021 23:03:38 GMT, Alexey Ivanov <[hidden email]> wrote:

>> BTW since the robot is not used it is unclear how the setUndecorated:true/false and setLocationRelativeTo() may affect the test.
>
> Since the text is rendered in JLabel and it's rendered into a BufferedImage, do we need the frame at all?
> If no frame is shown, the test can be headless, can't it?

It depends on what L&F needs to be tested. The Aqua for ex can be unavailable in headless.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2502
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8160720: [TEST_BUG] javax/swing/SwingUtilities/TestBadBreak/TestBadBreak.java

Alexey Ivanov-2
In reply to this post by Prasanta Sadhukhan-2
On Wed, 10 Feb 2021 10:33:03 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

> Test hardcodes bufferedimage and frame bounds so it does not capture correct size if scaling factor is used.
> The solution is to always use the same uiscale=1, an updated test still can reproduce the JDK-8015085 issue without fix. Additionally, moved the frame to centre of screen for better mach5 stability.
> Mach5 job running for several iterations on all platforms is ok. Link in JBS.

I think the test does something wrong. Now I see how `uiScale` affects rendering. The test does not paint JLabel directly to the image but rather in its overridden `paintComponent`. So `uiScale` controls the size of the component or rather where the break occurs.

For me on Windows with 200% scaling, in the first image ‘…’ is painted over the transparent background whereas in the second image ‘…’ is over the components opaque background.

The test passes with `uiScale=1` but I see no ‘…’ at all. Both images are clipped at “.1”.

As far as I understand, the purpose of the test is to make sure combining diacritics does not affect string clipping, and it occurs at the same position whether a composite character is used of a decomposed one. Yet no clipping occurs at all with `uiScale=1`. This looks wrong even though the test passes successfully. And when the test fails with `uiScale=2`, the images suggest there's a product bug because the string is clipped differently.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2502
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8160720: [TEST_BUG] javax/swing/SwingUtilities/TestBadBreak/TestBadBreak.java

Alexey Ivanov-2
On Thu, 11 Feb 2021 21:11:03 GMT, Alexey Ivanov <[hidden email]> wrote:

> For me on Windows with 200% scaling, in the first image ‘…’ is painted over the transparent background whereas in the second image ‘…’ is over the components opaque background.
>
> The test passes with `uiScale=1` but I see no ‘…’ at all. Both images are clipped at “.1”.

As you can see on the screenshots below, the strings are clipped differently. In the case where `uiScale=1`, the string is not clipped, it's displayed completely in the frame.
![TestBadBreak java-200](https://user-images.githubusercontent.com/70774172/107700672-bebbfe00-6caf-11eb-9f26-123405c9d6df.png)
![TestBadBreak java-100](https://user-images.githubusercontent.com/70774172/107700678-c1b6ee80-6caf-11eb-8840-697249eaf795.png)

Shall the test be modified to do some text measurement to ensure the JLabel cannot fit the text completely? And then paint it to the image of the calculated size?

As for the clipping itself, I think the first frame, the bottom one, looks correctly: the text is clipped right to the edge of the client area whereas the second frame, the upper one, leaves some whitespace.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2502
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8160720: [TEST_BUG] javax/swing/SwingUtilities/TestBadBreak/TestBadBreak.java

Alexey Ivanov-2
In reply to this post by Sergey Bylokhov-2
On Thu, 11 Feb 2021 19:49:21 GMT, Sergey Bylokhov <[hidden email]> wrote:

>> Since the text is rendered in JLabel and it's rendered into a BufferedImage, do we need the frame at all?
>> If no frame is shown, the test can be headless, can't it?
>
> It depends on what L&F needs to be tested. The Aqua for ex can be unavailable in headless.

Is it the point of the test? The test does not seem to depend on a particular L&F.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2502
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8160720: [TEST_BUG] javax/swing/SwingUtilities/TestBadBreak/TestBadBreak.java

Sergey Bylokhov-2
In reply to this post by Alexey Ivanov-2
On Thu, 11 Feb 2021 21:33:44 GMT, Alexey Ivanov <[hidden email]> wrote:

>> I think the test does something wrong. Now I see how `uiScale` affects rendering. The test does not paint JLabel directly to the image but rather in its overridden `paintComponent`. So `uiScale` controls the size of the component or rather where the break occurs.
>>
>> For me on Windows with 200% scaling, in the first image ‘…’ is painted over the transparent background whereas in the second image ‘…’ is over the components opaque background.
>>
>> The test passes with `uiScale=1` but I see no ‘…’ at all. Both images are clipped at “.1”.
>>
>> As far as I understand, the purpose of the test is to make sure combining diacritics does not affect string clipping, and it occurs at the same position whether a composite character is used of a decomposed one. Yet no clipping occurs at all with `uiScale=1`. This looks wrong even though the test passes successfully. And when the test fails with `uiScale=2`, the images suggest there's a product bug because the string is clipped differently.
>
>> For me on Windows with 200% scaling, in the first image ‘…’ is painted over the transparent background whereas in the second image ‘…’ is over the components opaque background.
>>
>> The test passes with `uiScale=1` but I see no ‘…’ at all. Both images are clipped at “.1”.
>
> As you can see on the screenshots below, the strings are clipped differently. In the case where `uiScale=1`, the string is not clipped, it's displayed completely in the frame.
> ![TestBadBreak java-200](https://user-images.githubusercontent.com/70774172/107700672-bebbfe00-6caf-11eb-9f26-123405c9d6df.png)
> ![TestBadBreak java-100](https://user-images.githubusercontent.com/70774172/107700678-c1b6ee80-6caf-11eb-8840-697249eaf795.png)
>
> Shall the test be modified to do some text measurement to ensure the JLabel cannot fit the text completely? And then paint it to the image of the calculated size?
>
> As for the clipping itself, I think the first frame, the bottom one, looks correctly: the text is clipped right to the edge of the client area whereas the second frame, the upper one, leaves some whitespace.

The rendering of the test may be affected by the uiScale, but when this test is executed, it uses only one-same scale for both use cases(different strings), and both of them should produce the same result.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2502
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8160720: [TEST_BUG] javax/swing/SwingUtilities/TestBadBreak/TestBadBreak.java

Alexey Ivanov-2
On Mon, 22 Feb 2021 18:34:42 GMT, Sergey Bylokhov <[hidden email]> wrote:

>
>
> The rendering of the test may be affected by the uiScale, but when this test is executed, it uses only one-same scale for both use cases(different strings), and both of them should produce the same result.

I compared the images produced by the test. The images are different in the same way as displayed on the screenshots above with 200%.

When using uiScale=1, the images are the same and no clipping occurs in this case. If no clipping occurs, the test does not test anything, does it?

The test has to ensure the string gets clipped so that ‘…’ replaces the part that does not fit in the size provided, and the result of clipping must be the same. This should not depend on the uiScale.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2502
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8160720: [TEST_BUG] javax/swing/SwingUtilities/TestBadBreak/TestBadBreak.java

Sergey Bylokhov-2
On Mon, 22 Feb 2021 18:45:38 GMT, Alexey Ivanov <[hidden email]> wrote:

> The test has to ensure the string gets clipped so that ‘…’ replaces the part that does not fit in the size provided, and the result of clipping must be the same. This should not depend on the uiScale.

The test tries to set the bounds of the window to the "special" size which trigger the clipping of only the last characters, if it fails to do so then it just check that the non-clipped text is the same for both strings. If 200% caused the test to fail because the wrong part of the text is clipped mean that this is not a test bug. And the change in the test should include using more scale/text variants to increase coverage, not to set it to the uiScale="1".

-------------

PR: https://git.openjdk.java.net/jdk/pull/2502
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8160720: [TEST_BUG] javax/swing/SwingUtilities/TestBadBreak/TestBadBreak.java

Alexey Ivanov-2
On Mon, 22 Feb 2021 19:17:29 GMT, Sergey Bylokhov <[hidden email]> wrote:

>
>
> > The test has to ensure the string gets clipped so that ‘…’ replaces the part that does not fit in the size provided, and the result of clipping must be the same. This should not depend on the uiScale.
>
> The test tries to set the bounds of the window to the "special" size which trigger the clipping of only the last characters, if it fails to do so then it just check that the non-clipped text is the same for both strings.

Since I cannot see the clipping occurs with uiScale=1, the test has to adjust its special size, probably by measuring the text before setting the size.

> If 200% caused the test to fail because the wrong part of the text is clipped mean that this is not a test bug. And the change in the test should include using more scale/text variants to increase coverage, not to set it to the uiScale="1".

That's exactly my point. The current behaviour looks like a product bug. So there's more to it than just setting uiScale=1, which makes the test pass but then the test does not clip the string at all as we discussed above.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2502
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8160720: [TEST_BUG] javax/swing/SwingUtilities/TestBadBreak/TestBadBreak.java

Sergey Bylokhov-2
On Mon, 22 Feb 2021 19:37:17 GMT, Alexey Ivanov <[hidden email]> wrote:

> Since I cannot see the clipping occurs with uiScale=1, the test has to adjust its special size, probably by measuring the text before setting the size.

I have retested it w/o the fix for JDK-8015085 on the 100% dpi system(w/o uscale option) and the test fails w/o the fix and passed with, so it verify something.

Need to check what goes wrong in the HiDPI case.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2502
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8160720: [TEST_BUG] javax/swing/SwingUtilities/TestBadBreak/TestBadBreak.java

Alexey Ivanov-2
On Mon, 22 Feb 2021 20:02:30 GMT, Sergey Bylokhov <[hidden email]> wrote:

> > Since I cannot see the clipping occurs with uiScale=1, the test has to adjust its special size, probably by measuring the text before setting the size.
>
> I have retested it w/o the fix for JDK-8015085 on the 100% dpi system(w/o uscale option) and the test fails w/o the fix and passed with, so it verify something.

I have run a couple of tests on system with 100% scaling. The unmodified test passes there and it performs the test as intended:
![TestBadBreak@100%](https://user-images.githubusercontent.com/70774172/108901738-2872cb00-7613-11eb-8574-595c3fc80765.png)

As you can see, the clipping occurs not at the last character, however, it's not required after all.

Yet I cannot explain why uiScale=1 produces different result on the system with 200% scaling. Off the top of my head, the minimum width of the window could be larger on HiDPI system.


> Need to check what goes wrong in the HiDPI case.

But the test fails in HiDPI case. If I use uiScale=2 on the system with 100%, the result is exactly the same as on the system with 200%. If you change the size of the frames manually, the combining case works as expected: as soon as the string does not fit in the width, it gets clipped, two characters “/ä” (or rather two glyphs) are replaced with “…”. But if you slowly change the width of the frame with precomposed character, three characters “3/ä” disappear; if you continue shrinking the frame, there's always a gap between the “…” and the edge of the frame. Yet in the other frame, another character gets clipped only when “…” touches the edge. (I used uiScale=4 for testing, it makes the font larger and it's easier to see the difference.)

So, this is a product bug.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2502