On Tue, 11 Oct 2022 14:17:42 GMT, Manukumar V S <[email protected]> wrote:
> java/awt/PopupMenu/PopupMenuLocation.java seems to be unstable in MacOS
> machines, especially in MacOSX 12 & 13 machines. It seems to be a testbug as
> adding some stability improvements fixes the issue. It intermittently fails
> in CI causing some noise. This test was already problem listed in windows due
> to an umbrella bug JDK-8238720. This fix doesn't cover the Windows issue, so
> the problem listing in windows will remain same.
>
> Fix:
> Some stability improvements have been done and the test has been run 100
> times per platform in mach5 and got full PASS.
> Also updated the screen capture code, made frame undecorated, added some
> prints for better debugging.
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/PopupMenu/PopupMenuLocation.java line 42:
> 40: import java.util.stream.IntStream;
> 41: import javax.imageio.ImageIO;
> 42: import javax.swing.SwingUtilities;
Remove the unused imports: `AtomicReference` and `SwingUtilities`.
test/jdk/java/awt/PopupMenu/PopupMenuLocation.java line 59:
> 57: private static Robot robot;
> 58: private static Frame frame;
> 59: private static Rectangle currentScreenBounds;
Suggestion:
private static Rectangle screenBounds;
I think it's still clear enough yet shorter.
test/jdk/java/awt/PopupMenu/PopupMenuLocation.java line 72:
> 70: currentScreenBounds = gc.getBounds();
> 71: Point point = new Point(currentScreenBounds.x,
> currentScreenBounds.y);
> 72: Insets insets =
> Toolkit.getDefaultToolkit().getScreenInsets(gc);
Suggestion:
Insets insets = Toolkit.getDefaultToolkit().getScreenInsets(gc);
Point point = new Point(currentScreenBounds.x + insets.left,
currentScreenBounds.y + insets.top);
The initial position of the frame should take the insets into account.
test/jdk/java/awt/PopupMenu/PopupMenuLocation.java line 129:
> 127: Point pt = frame.getLocationOnScreen();
> 128: int x = pt.x + frame.getWidth() / 2;
> 129: int y = pt.y + 50;
I suggest creating a constant `OFFSET` with the value of `50`. It would allow
changing the position of the click easily.
test/jdk/java/awt/PopupMenu/PopupMenuLocation.java line 143:
> 141: throw new RuntimeException(
> 142: "Failed, Not received the PopupMenu ActionEvent yet
> on " +
> 143: "frame= "+frame+", isFocused = "+frame.isFocused());
Suggestion:
"Failed, didn't receive the PopupMenu ActionEvent on " +
"frame= " + frame + ", isFocused = " + frame.isFocused());
Grammar and spaces around the `+` operator.
test/jdk/java/awt/PopupMenu/PopupMenuLocation.java line 153:
> 151: new File("screen1.png"));
> 152: } catch (Exception exception) {
> 153: exception.printStackTrace();
Suggestion:
} catch (Exception e) {
e.printStackTrace();
Why not use the standard name for the exception variable? Spelling the entire
word doesn't add any value.
test/jdk/java/awt/PopupMenu/PopupMenuLocation.java line 155:
> 153: exception.printStackTrace();
> 154: }
> 155: action = false;
Suggestion:
It's unneeded here and rather confusing.
-------------
PR: https://git.openjdk.org/jdk/pull/10655
