On Fri, 13 Feb 2026 15:36:50 GMT, Alexey Ivanov <[email protected]> wrote:
>> Khalid Boulanouare has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Re problem list unstable test > > test/jdk/java/awt/Mixing/AWT_Mixing/GlassPaneOverlappingTestBase.java line > 133: > >> 131: testedComponent.setBounds(0, 0, >> 132: >> testedComponent.getPreferredSize().width, >> 133: >> testedComponent.getPreferredSize().height + 20); > > This added code fully duplicates the existing code at lines > [167–184](https://github.com/kboulanou/jdk/blob/a2b9a388fbcf836de159705500bdb22cedb2d1d9/test/jdk/java/awt/Mixing/AWT_Mixing/GlassPaneOverlappingTestBase.java#L167-L184) > except for focus handling. Modify the code below. > > Otherwise, lines 167–184 are unreachable. Code modified as requested. > test/jdk/java/awt/Mixing/AWT_Mixing/GlassPaneOverlappingTestBase.java line > 135: > >> 133: >> testedComponent.getPreferredSize().height + 20); >> 134: Component focusOwner = >> KeyboardFocusManager.getCurrentKeyboardFocusManager() >> 135: .getFocusOwner(); > > Suggestion: > > Component focusOwner = KeyboardFocusManager > > .getCurrentKeyboardFocusManager() > .getFocusOwner(); > > What do you think about this way? The line isn't as long. Looks good to me. I updated code accordingly. > test/jdk/java/awt/Mixing/AWT_Mixing/GlassPaneOverlappingTestBase.java line > 148: > >> 146: } >> 147: Point lLoc = testedComponent.getLocationOnScreen(); >> 148: lLoc.translate(1, testedComponent.getPreferredSize().height >> + 1); > > `getLocationOnScreen` and `getPreferredSize` are better called on EDT, > however, I think it's safe here. You've got a synchronisation point with > `invokeAndWait`, neither `getLocationOnScreen` and `getPreferredSize` could > change, therefore the returned values would be consistent. Tasks run in EDT, please have a look. Thanks. > test/jdk/java/awt/Mixing/AWT_Mixing/GlassPaneOverlappingTestBase.java line > 165: > >> 163: >> 164: return true; >> 165: } > > This looks weird. I'd separate the logical blocks above with blank lines, but > here the blank is absolutely redundant. > > Moreover, it seems to me `if (!resize)` should've remained above. That is if > `!resize` we don't need the latch, we don't need anything because the test is > skipped. So keep it that way: exit from the method early than later. > > Everything the follows `if (!resize)` block is where the action is happening. > > In other words, what I suggest is > > > protected boolean performTest() { > if (!super.performTest()) { > return false; > } > > if (!testResize) { > return true; > } > > // The above piece of code remains unchanged > > // Create the latch and handle focus here > // ... I have re-ordered the sequence of execution here. Please have a look. Thanks. > test/jdk/java/awt/Mixing/AWT_Mixing/JComboBoxOverlapping.java line 29: > >> 27: import java.awt.event.ActionEvent; >> 28: import java.awt.event.ActionListener; >> 29: import java.lang.Override; > > No need to explicitly import anything from the `java.lang` package. I have removed this unnecess ary import from this class and other classes. > test/jdk/java/awt/Mixing/AWT_Mixing/JComboBoxOverlapping.java line 70: > >> 68: frame.getContentPane().setLayout(new >> BoxLayout(frame.getContentPane(), BoxLayout.Y_AXIS)); >> 69: frame.setSize(200, 200); >> 70: cb = new JComboBox(petStrings); > > I prefer to keep this blank line, it separates creating the frame and setting > its size from creating and configuring the combo box. I have restored back this space. > test/jdk/java/awt/Mixing/AWT_Mixing/JComboBoxOverlapping.java line 104: > >> 102: >> 103: loc2.translate(75, 75); >> 104: robot.mouseMove(0, 0); > > I believe this line is asking for comment: > > > Suggestion: > > robot.mouseMove(0, 0); // Avoid capturing mouse cursor Comment added as requested. > test/jdk/java/awt/Mixing/AWT_Mixing/JMenuBarOverlapping.java line 32: > >> 30: import java.awt.event.MouseAdapter; >> 31: import java.awt.event.MouseEvent; >> 32: import java.lang.Override; > > Suggestion: > > > Redundant import. Unnecessary comment removed. > test/jdk/java/awt/Mixing/AWT_Mixing/JMenuBarOverlapping.java line 79: > >> 77: frame.setLocationRelativeTo(null); >> 78: frame.setVisible(true); >> 79: menuBar = new JMenuBar(); > > Suggestion: > > frame.setLocationRelativeTo(null); > frame.setVisible(true); > > menuBar = new JMenuBar(); > > Preserve the blank line. Blank line restored as requested. > test/jdk/java/awt/Mixing/AWT_Mixing/JMenuBarOverlapping.java line 110: > >> 108: propagateAWTControls(frame); >> 109: >> 110: } > > Suggestion: > > } > > A blank line right before the closing brace is redundant. Blank line added. > test/jdk/java/awt/Mixing/AWT_Mixing/JMenuBarOverlapping.java line 126: > >> 124: Robot robot = Util.createRobot(); >> 125: robot.setAutoDelay(ROBOT_DELAY); >> 126: robot.mouseMove(0, 0); > > A comment would be nice. Comment added as requested. > test/jdk/java/awt/Mixing/AWT_Mixing/JPopupMenuOverlapping.java line 92: > >> 90: frame.setVisible(true); >> 91: loc = frame.getContentPane().getLocationOnScreen(); >> 92: } > > You removed `setLocationRelativeTo` and `setVisible` in > `JMenuBarOverlapping.java`, should these be removed too? > > `setLocationRelativeTo` is redundant here, as you already called it on line > 73. setLocationRelativeTo and setVisible removed as requested. > test/jdk/java/awt/Mixing/AWT_Mixing/JPopupMenuOverlapping.java line 99: > >> 97: Robot robot = Util.createRobot(); >> 98: robot.setAutoDelay(ROBOT_DELAY); >> 99: robot.mouseMove(0, 0); > > I'd add a comment to explain moving to (0, 0). Comment added. > test/jdk/java/awt/Mixing/AWT_Mixing/JPopupMenuOverlapping.java line 104: > >> 102: >> 103: pixelPreCheck(robot, loc, currentAwtControl); >> 104: try { > > If removing a blank line, I'd rather remove the one between `translate` and > `pixelPreCheck` instead of this. Those calls are closer related than the > `try` block below. Blank line restored and removed blank line between `translate` and `pixelPreCheck`. > test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 61: > >> 59: public class MixingPanelsResizing { >> 60: >> 61: static final int TOLERANCE_MACOSX = 15; > > Suggestion: > > private static final int TOLERANCE_MACOSX = 15; > > For consistency with other declarations. Field accessor added as requested. > test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 62: > >> 60: >> 61: static final int TOLERANCE_MACOSX = 15; >> 62: static volatile boolean failed = false; > > The field `failed` in unused, it can be removed then. Unnecessary field removed as requested. > test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 119: > >> 117: Math.abs(borderShift) == 1 ? >> 118: borderShift : >> 119: (borderShift / 2); > > Suggestion: > > borderShift = Math.abs(borderShift) == 1 > ? borderShift > : (borderShift / 2); > > I prefer this formatting. Yet we don't have a style guide to follow. > > In general, wrapping before the operator makes it clearer that it's a > continuation line. Formatting updated as requested. > test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 169: > >> 167: SwingUtilities.invokeAndWait(new Runnable() { >> 168: >> 169: public void run() { > > Suggestion: > > SwingUtilities.invokeAndWait(new Runnable() { > public void run() { > > I'd rather not add this line… On the other hand, it's more or less consistent > with other cases where the `Runnable` interface is implemented. Blank line removed as requested. > test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 255: > >> 253: public static void main(String args[]) throws Exception { >> 254: >> 255: if (!Toolkit.getDefaultToolkit().isDynamicLayoutActive()) { > > I'm strongly against adding blank lines at the start of a method. The > declaration of a method serves as the start of a new logical block by itself. I have removed blank line as requested. > test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 277: > >> 275: //Timed out, so fail the test >> 276: throw new RuntimeException( >> 277: "Timed out after " + sleepTime / 1000 + " seconds"); > > Suggestion: > > "Timed out after " + (sleepTime / 1000) + " seconds"); > > Parentheses aren't required here, yet they help avoid any ambiguity. Formatting updated as requested. > test/jdk/java/awt/Mixing/AWT_Mixing/OpaqueOverlapping.java line 1: > >> 1: /* > > If there's nothing else modified in the `OpaqueOverlapping.java` file, I'd > rather revert these changes and leave the file untouched, moreover importing > `java.lang.Override` is not needed. File restored from previous version. > test/jdk/java/awt/Mixing/AWT_Mixing/OverlappingTestBase.java line 32: > >> 30: import java.awt.Dimension; >> 31: import java.awt.Font; >> 32: import java.awt.Helper; > > I'd probably put the helper into the section of helper classes… but this > means that imports need manual editing after IDE updates them for you. So, > probably leave it as is now. I see, I have left it as it is. Thanks > test/jdk/java/awt/Mixing/AWT_Mixing/OverlappingTestBase.java line 47: > >> 45: import java.io.InputStream; >> 46: import java.io.InputStreamReader; >> 47: import java.lang.Override; > > No need to import `Override`. Unnecessary import removed. > test/jdk/java/awt/Mixing/AWT_Mixing/OverlappingTestBase.java line 389: > >> 387: return !(component == null || >> 388: (component instanceof Scrollbar) || >> 389: (isMac && (component instanceof Button))); > > Suggestion: > > return !(component == null > || (component instanceof Scrollbar) > || (isMac && (component instanceof Button))); > > Wrap before the operator to clearly indicate a continuation line. Operators location updated as requested. > test/jdk/java/awt/Mixing/AWT_Mixing/OverlappingTestBase.java line 389: > >> 387: return !(component == null || >> 388: (component instanceof Scrollbar) || >> 389: (isMac && (component instanceof Button))); > > Does inverting the condition makes it clearer? > > > return component != null > && !(component instanceof Scrollbar) > && !(isMac && (component instanceof Button)); > > That is verify any non-null component except `Scrollbar`, and in addition > `Button` on macOS. Code update as requested. > test/jdk/java/awt/Mixing/AWT_Mixing/SimpleOverlappingTestBase.java line 161: > >> 159: JFrame ancestor = (JFrame) >> (testedComponent.getTopLevelAncestor()); >> 160: final CountDownLatch latch = new CountDownLatch(1); >> 161: if (ancestor != null) { > > Suggestion: > > final CountDownLatch latch = new CountDownLatch(1); > > JFrame ancestor = (JFrame) (testedComponent.getTopLevelAncestor()); > if (ancestor != null) { > > I suggest moving the declarations around. The `latch` is helper object. You > then get the ancestor and work with it. Order of execution updated as requested. > test/jdk/java/awt/Mixing/AWT_Mixing/SimpleOverlappingTestBase.java line 171: > >> 169: latch.countDown(); >> 170: } >> 171: try { > > Add a blank line between the `if` statement and `try`. The `if` block > prepares for the code inside `try`, they're related, but still do separate > job. > > Reading code that separates logical blocks with blank lines is easier. Blank line added as requested. > test/jdk/java/awt/Mixing/AWT_Mixing/SimpleOverlappingTestBase.java line 173: > >> 171: try { >> 172: boolean await = latch.await(1, TimeUnit.SECONDS); >> 173: if (!await) { > > Suggestion: > > if (!latch.await(1, TimeUnit.SECONDS)) { > > The usage of a local variable doesn't add clarity. Unnecessary local variable removed as suggested. > test/jdk/java/awt/Mixing/AWT_Mixing/SimpleOverlappingTestBase.java line 181: > >> 179: throw new RuntimeException(e); >> 180: } >> 181: if (ancestor != null && isMultiFramesTest()) { > > Move the call to `clickAndBlink` back here along with the blank line. > > You [say](https://github.com/openjdk/jdk/pull/26625/changes#r2650824338), > “`clickAndBlink` happens only when we get focus”, which is directly after the > `try` block. If the focus isn't received, an exception is thrown from the > `try` block. Order of execution updated as requested. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816923053 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816931726 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816928616 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816921525 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816938618 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816941556 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816942827 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816989616 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816944949 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816947178 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816988145 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816953294 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816948090 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816951576 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816957213 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816954624 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816958353 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816960534 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816986266 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816962036 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816983209 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816998102 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816963085 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816965367 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816968056 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816970509 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816980813 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816979573 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816977810
