From f57716f23b770326f67f5512fa677e766b5397fd Mon Sep 17 00:00:00 2001 From: jeremy Date: Wed, 14 May 2025 23:58:09 -0400 Subject: [PATCH 1/4] 8356061: slowing down RootPaneDefaultButtonTest I could reproduce failures with this test if I changed every call to Robot.delay to 1ms. (I could not reproduce failures from the master branch on my machine.) I'm increasing the delay before we start capturing pixels from 100ms to 1000ms. --- test/jdk/com/apple/laf/RootPane/RootPaneDefaultButtonTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/jdk/com/apple/laf/RootPane/RootPaneDefaultButtonTest.java b/test/jdk/com/apple/laf/RootPane/RootPaneDefaultButtonTest.java index f314d48841c81..96307db256a23 100644 --- a/test/jdk/com/apple/laf/RootPane/RootPaneDefaultButtonTest.java +++ b/test/jdk/com/apple/laf/RootPane/RootPaneDefaultButtonTest.java @@ -128,7 +128,7 @@ private static void test(Robot robot, AbstractButton buttonToClick, robot.delay(20); robot.mouseRelease(InputEvent.BUTTON1_DOWN_MASK); - robot.delay(100); + robot.delay(1000); // the colors may change depending on your system's appearance. // Depending on how you've configured "Appearance" in the From b7270924aab74ee0ef3390954cd10e5a22022b8f Mon Sep 17 00:00:00 2001 From: jeremy Date: Thu, 15 May 2025 00:00:16 -0400 Subject: [PATCH 2/4] 8356061: additional output in the event of failure This may be a red herring, but I think it's weird that the test failed because it spotted instances of the color 0x373737. I don't know exactly where that would appear in the test app's UI. --- .../RootPane/RootPaneDefaultButtonTest.java | 60 +++++++++++-------- 1 file changed, 34 insertions(+), 26 deletions(-) diff --git a/test/jdk/com/apple/laf/RootPane/RootPaneDefaultButtonTest.java b/test/jdk/com/apple/laf/RootPane/RootPaneDefaultButtonTest.java index 96307db256a23..d1f5657fbf8bd 100644 --- a/test/jdk/com/apple/laf/RootPane/RootPaneDefaultButtonTest.java +++ b/test/jdk/com/apple/laf/RootPane/RootPaneDefaultButtonTest.java @@ -139,39 +139,47 @@ private static void test(Robot robot, AbstractButton buttonToClick, Color defaultColor = null; Color nonDefaultColor = null; - for (ButtonRenderingExpectation expectation : expectations) { - int x = expectation.button.getLocationOnScreen().x + 20; - int y = expectation.button.getLocationOnScreen().y + 10; - - // this mouseMove is optional, but it helps debug this test to see - // where we're sampling the pixel color from: - robot.mouseMove(x, y); - - Color c = robot.getPixelColor(x, y); - if (expectation.appearAsDefault) { - if (defaultColor == null) { - defaultColor = c; + for (int a = 0; a < expectations.length; a++) { + try { + ButtonRenderingExpectation expectation = expectations[a]; + int x = expectation.button.getLocationOnScreen().x + 20; + int y = expectation.button.getLocationOnScreen().y + 10; + + // this mouseMove is optional, but it helps debug this test to see + // where we're sampling the pixel color from: + robot.mouseMove(x, y); + + Color c = robot.getPixelColor(x, y); + if (expectation.appearAsDefault) { + if (defaultColor == null) { + defaultColor = c; + } else { + throw new IllegalStateException( + "there should only be at most 1 default button"); + } } else { - throw new IllegalStateException( - "there should only be at most 1 default button"); + if (nonDefaultColor == null) { + nonDefaultColor = c; + } else if (!isSimilar(nonDefaultColor, c)) { + throw new IllegalStateException( + "these two colors should match: " + c + ", " + + nonDefaultColor); + } } - } else { - if (nonDefaultColor == null) { - nonDefaultColor = c; - } else if (!isSimilar(nonDefaultColor, c)) { + + if (defaultColor != null && nonDefaultColor != null && + isSimilar(defaultColor, nonDefaultColor)) { throw new IllegalStateException( - "these two colors should match: " + c + ", " + + "The default button and non-default buttons should " + + "look different: " + defaultColor + " matches " + nonDefaultColor); } + } catch(Exception e) { + System.err.println("a = " + a + " defaultColor = " + + defaultColor + " nonDefaultColor = " + nonDefaultColor); + throw e; } } - - if (defaultColor != null && isSimilar(defaultColor, nonDefaultColor)) { - throw new IllegalStateException( - "The default button and non-default buttons should " + - "look different: " + defaultColor + " matches " + - nonDefaultColor); - } } private static boolean isSimilar(Color c1, Color c2) { From 9b926904fa0aa3ab44f6b809dbf632760b36ed78 Mon Sep 17 00:00:00 2001 From: jeremy Date: Thu, 15 May 2025 14:59:32 -0400 Subject: [PATCH 3/4] 8356061: adding 1000ms pause after window construction This was requested here: https://github.com/openjdk/jdk/pull/25244#discussion_r2091656355 So now we'll pause at least 1.1s before the first call to `jc.getLocationOnScreen`, and at least 2.1s before the first call to `robot.getPixelColor(x, y)`. (getLocationOnScreen has never failed with an IllegalStateException.) --- test/jdk/com/apple/laf/RootPane/RootPaneDefaultButtonTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/test/jdk/com/apple/laf/RootPane/RootPaneDefaultButtonTest.java b/test/jdk/com/apple/laf/RootPane/RootPaneDefaultButtonTest.java index d1f5657fbf8bd..602491ad33544 100644 --- a/test/jdk/com/apple/laf/RootPane/RootPaneDefaultButtonTest.java +++ b/test/jdk/com/apple/laf/RootPane/RootPaneDefaultButtonTest.java @@ -74,6 +74,7 @@ public void run() { window2.setVisible(true); } }); + robot.delay(1000); Robot robot = new Robot(); From b076c96425c5a0a66f8f25d081b67b6028dc9bd7 Mon Sep 17 00:00:00 2001 From: jeremy Date: Thu, 15 May 2025 15:19:02 -0400 Subject: [PATCH 4/4] 8356061: restructuring to move things to EDT This was requested here: https://github.com/openjdk/jdk/pull/25244#discussion_r2091656355 mrserb asked: "Also please move creation and access_to all Swing components to EDT" (I'm not sure this will help much? My understanding was *creation* of Swing components could happen off the EDT as long as they were made displayable on the EDT.) Now we still call jc.getLocationOnScreen off the EDT. If that posed a thread-based problem it'd probably manifest as a IllegalComponentStateException, which is not mentioned in 8356061. --- .../RootPane/RootPaneDefaultButtonTest.java | 198 ++++++++++-------- 1 file changed, 106 insertions(+), 92 deletions(-) diff --git a/test/jdk/com/apple/laf/RootPane/RootPaneDefaultButtonTest.java b/test/jdk/com/apple/laf/RootPane/RootPaneDefaultButtonTest.java index 602491ad33544..c5fe4998d25b7 100644 --- a/test/jdk/com/apple/laf/RootPane/RootPaneDefaultButtonTest.java +++ b/test/jdk/com/apple/laf/RootPane/RootPaneDefaultButtonTest.java @@ -34,6 +34,7 @@ import javax.swing.border.*; import java.awt.*; import java.awt.event.*; +import java.lang.reflect.InvocationTargetException; /** * This presents two dialogs, each with two possible default buttons. The @@ -45,38 +46,124 @@ * to double-check that the removed code didn't negatively affect how default * buttons are repainted. */ -public class RootPaneDefaultButtonTest extends JDialog { +public class RootPaneDefaultButtonTest { record ButtonRenderingExpectation(JButton button, boolean appearAsDefault) {} + static class TestDialog extends JDialog { + JRadioButton radioButton1 = new JRadioButton( + "\"Button 1\" is the default button"); + JRadioButton radioButton2 = new JRadioButton( + "\"Button 2\" is the default button"); + JRadioButton radioButton3 = new JRadioButton("No default button"); + + JButton button1 = new JButton("Button 1"); + JButton button2 = new JButton("Button 2"); + + TestDialog() { + getContentPane().setLayout(new BorderLayout()); + getContentPane().add(createRadioButtonPanel(), BorderLayout.NORTH); + getContentPane().add(createPushButtonRow(), BorderLayout.SOUTH); + pack(); + + radioButton1.addActionListener(new ActionListener() { + @Override + public void actionPerformed(ActionEvent e) { + getRootPane().setDefaultButton(button1); + } + }); + + radioButton2.addActionListener(new ActionListener() { + @Override + public void actionPerformed(ActionEvent e) { + getRootPane().setDefaultButton(button2); + } + }); + + radioButton3.addActionListener(new ActionListener() { + @Override + public void actionPerformed(ActionEvent e) { + getRootPane().setDefaultButton(null); + } + }); + + ButtonGroup g = new ButtonGroup(); + g.add(radioButton1); + g.add(radioButton2); + g.add(radioButton3); + radioButton1.doClick(); + } + + private JPanel createPushButtonRow() { + JPanel p = new JPanel(new GridLayout(1, 2)); + p.add(button1); + p.add(button2); + p.setBorder(new EmptyBorder(5,5,5,5)); + return p; + } + + private JPanel createRadioButtonPanel() { + JPanel p = new JPanel(new GridLayout(3, 1)); + p.add(radioButton1); + p.add(radioButton2); + p.add(radioButton3); + p.setBorder(new EmptyBorder(5,5,5,5)); + return p; + } + } + + private static boolean isSimilar(Color c1, Color c2) { + if (Math.abs(c1.getRed() - c2.getRed()) > 15) { + return false; + } + if (Math.abs(c1.getGreen() - c2.getGreen()) > 15) { + return false; + } + if (Math.abs(c1.getBlue() - c2.getBlue()) > 15) { + return false; + } + return true; + } + public static void main(String[] args) throws Exception { if (!System.getProperty("os.name").contains("OS X")) { System.out.println("This test is for MacOS only."); return; } - RootPaneDefaultButtonTest window1 = new RootPaneDefaultButtonTest(); - RootPaneDefaultButtonTest window2 = new RootPaneDefaultButtonTest(); + RootPaneDefaultButtonTest test = new RootPaneDefaultButtonTest(); + test.run(); + } - SwingUtilities.invokeAndWait(new Runnable() { - @Override - public void run() { - Rectangle r1 = new Rectangle(0, 20, - window1.getWidth(), window1.getHeight()); - window1.setBounds(r1); + TestDialog window1, window2; + Robot robot; - Rectangle r2 = new Rectangle((int) (r1.getMaxX() + 10), 20, - window2.getWidth(), window2.getHeight()); - window2.setBounds(r2); + public RootPaneDefaultButtonTest() throws Exception { + SwingUtilities.invokeAndWait(() -> { + window1 = new TestDialog(); + window2 = new TestDialog(); - window1.setVisible(true); - window2.setVisible(true); - } + Rectangle r1 = new Rectangle(0, 20, + window1.getWidth(), window1.getHeight()); + window1.setBounds(r1); + + Rectangle r2 = new Rectangle((int) (r1.getMaxX() + 10), 20, + window2.getWidth(), window2.getHeight()); + window2.setBounds(r2); + + window1.setVisible(true); + window2.setVisible(true); }); - robot.delay(1000); + robot = new Robot(); + } + - Robot robot = new Robot(); + /** + * This is not run on the EDT; once it finishes this test is complete. + */ + private void run() throws Exception { + robot.delay(1000); test(robot, window1.radioButton1, new ButtonRenderingExpectation(window1.button1, true), @@ -117,8 +204,8 @@ public void run() { System.out.println("Test passed successfully"); } - private static void test(Robot robot, AbstractButton buttonToClick, - ButtonRenderingExpectation... expectations) + private void test(Robot robot, AbstractButton buttonToClick, + ButtonRenderingExpectation... expectations) throws Exception { robot.delay(100); @@ -182,77 +269,4 @@ private static void test(Robot robot, AbstractButton buttonToClick, } } } - - private static boolean isSimilar(Color c1, Color c2) { - if (Math.abs(c1.getRed() - c2.getRed()) > 15) { - return false; - } - if (Math.abs(c1.getGreen() - c2.getGreen()) > 15) { - return false; - } - if (Math.abs(c1.getBlue() - c2.getBlue()) > 15) { - return false; - } - return true; - } - - JRadioButton radioButton1 = new JRadioButton( - "\"Button 1\" is the default button"); - JRadioButton radioButton2 = new JRadioButton( - "\"Button 2\" is the default button"); - JRadioButton radioButton3 = new JRadioButton("No default button"); - - JButton button1 = new JButton("Button 1"); - JButton button2 = new JButton("Button 2"); - - public RootPaneDefaultButtonTest() { - getContentPane().setLayout(new BorderLayout()); - getContentPane().add(createRadioButtonPanel(), BorderLayout.NORTH); - getContentPane().add(createPushButtonRow(), BorderLayout.SOUTH); - pack(); - - radioButton1.addActionListener(new ActionListener() { - @Override - public void actionPerformed(ActionEvent e) { - getRootPane().setDefaultButton(button1); - } - }); - - radioButton2.addActionListener(new ActionListener() { - @Override - public void actionPerformed(ActionEvent e) { - getRootPane().setDefaultButton(button2); - } - }); - - radioButton3.addActionListener(new ActionListener() { - @Override - public void actionPerformed(ActionEvent e) { - getRootPane().setDefaultButton(null); - } - }); - - ButtonGroup g = new ButtonGroup(); - g.add(radioButton1); - g.add(radioButton2); - g.add(radioButton3); - radioButton1.doClick(); - } - - private JPanel createPushButtonRow() { - JPanel p = new JPanel(new GridLayout(1, 2)); - p.add(button1); - p.add(button2); - p.setBorder(new EmptyBorder(5,5,5,5)); - return p; - } - - private JPanel createRadioButtonPanel() { - JPanel p = new JPanel(new GridLayout(3, 1)); - p.add(radioButton1); - p.add(radioButton2); - p.add(radioButton3); - p.setBorder(new EmptyBorder(5,5,5,5)); - return p; - } }