On Sun, 24 Sep 2023 06:12:17 GMT, Alisen Chung <ach...@openjdk.org> wrote:
>> Opening closed tests:
>> 12 javax/swing/ToolTipManager/5078214/bug5078214.java
>> 13 javax/swing/plaf/basic/BasicMenuItemUI/4239714/bug4239714.java
>> 14 javax/swing/plaf/basic/BasicMenuUI/4244616/bug4244616.java
>> 15 javax/swing/plaf/metal/4306431/bug4306431.java
>
> Alisen Chung has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - updated tests based on feedback
> - updated tests
Changes requested by aivanov (Reviewer).
test/jdk/javax/swing/BasicMenuUI/bug4244616.java line 67:
> 65: action.actionPerformed(ev);
> 66: } catch (Exception ignored) {
> 67: }
Are exceptions thrown here?
test/jdk/javax/swing/BasicMenuUI/bug4244616.java line 72:
> 70: // Restore streams, check results
> 71: System.setOut(oldOut);
> 72: System.setErr(oldErr);
This should be in `finally` block.
test/jdk/javax/swing/BasicMenuUI/bug4244616.java line 76:
> 74: if (bout.size() != 0 || berr.size() != 0) {
> 75: throw new RuntimeException("Failed: some debug output
> occurred");
> 76: }
Does it make sense to output the contents of `bout` and `berr`?
test/jdk/javax/swing/LookAndFeel/bug4306431.java line 37:
> 35:
> 36: public class bug4306431 {
> 37: static Font FONT = new Font(Font.MONOSPACED, Font.ITALIC, 24);
Suggestion:
static final Font FONT = new Font(Font.MONOSPACED, Font.ITALIC, 24);
test/jdk/javax/swing/LookAndFeel/bug4306431.java line 48:
> 46: }
> 47:
> 48: static class MetalTheme extends DefaultMetalTheme {
I suggest renaming `MetalTheme` to `TestMetalTheme` so that, when you read the
main method, you know right away it's not one of the default themes but a
custom test theme.
test/jdk/javax/swing/ToolTipManager/bug5078214.java line 47:
> 45: import javax.swing.JFrame;
> 46: import javax.swing.SwingUtilities;
> 47: import javax.swing.ToolTipManager;
Suggestion:
import java.awt.BorderLayout;
import java.awt.GraphicsConfiguration;
import java.awt.GraphicsDevice;
import java.awt.GraphicsEnvironment;
import java.awt.Insets;
import java.awt.Point;
import java.awt.Rectangle;
import java.awt.Toolkit;
import java.awt.Window;
import java.awt.event.MouseEvent;
import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.SwingUtilities;
import javax.swing.ToolTipManager;
Imports should be sorted. Your IDE will do it for you.
`java.awt.event.MouseEvent` should go after `java.awt.Window` rather than
`java.awt.BorderLayout`.
test/jdk/javax/swing/ToolTipManager/bug5078214.java line 63:
> 61: getLocalGraphicsEnvironment();
> 62: GraphicsDevice[] gs = ge.getScreenDevices();
> 63: GraphicsConfiguration testGraphics = null;
Suggestion:
GraphicsConfiguration testGraphicsConfig = null;
The name `testGraphics` suggests you're about to test `Graphics` object.
I would suggest moving the code which determines the `GraphicsConfiguration` to
test into a helper method. And this helper method can safely be run on the main
thread.
test/jdk/javax/swing/ToolTipManager/bug5078214.java line 69:
> 67: gd.getConfigurations();
> 68: for (int i = 0; i < gc.length; i++) {
> 69: insets =
> Toolkit.getDefaultToolkit().getScreenInsets(gc[i]);
Shouldn't it use the default configuration of the graphics device? The test
does nothing to activate a configuration.
test/jdk/javax/swing/ToolTipManager/bug5078214.java line 111:
> 109: test(bounds, insets);
> 110: r.waitForIdle();
> 111: r.delay(1000);
Any delays after you verified the condition for failure make no sense — the
value of `passed` will not change, remove all the delays after `test`.
test/jdk/javax/swing/ToolTipManager/bug5078214.java line 126:
> 124:
> 125: public static void test(Rectangle b, Insets i) {
> 126: r.delay(500);
There was a delay of 1,000 milliseconds just before calling `test`, isn't it
enough?
test/jdk/javax/swing/ToolTipManager/bug5078214.java line 128:
> 126: r.delay(500);
> 127:
> ToolTipManager.sharedInstance().setLightWeightPopupEnabled(false);
> 128: ToolTipManager.sharedInstance().setInitialDelay(100);
Shouldn't `ToolTipManager` be accessed on EDT?
Since you [_“agreed to keep components on
EDT”_](https://github.com/openjdk/jdk/pull/15755#discussion_r1329183236)
(according to @DamonGuy), `mainFrame.getOwnedWindows()` should also be called
on EDT.
test/jdk/javax/swing/ToolTipManager/bug5078214.java line 136:
> 134: passed = true;
> 135: return;
> 136: }
To me, this is a failure.
Perhaps, delay of 2 seconds is too long. You explicitly set the tooltip delay
to 100ms, so the tooltip should display in about this time. Waiting 2 seconds
could lead to the tooltip being hidden.
test/jdk/javax/swing/ToolTipManager/bug5078214.java line 143:
> 141: // Position is Ok!
> 142: passed = true;
> 143: }
Suggestion:
passed = (wy < (b.y + b.height - i.bottom));
-------------
PR Review: https://git.openjdk.org/jdk/pull/15875#pullrequestreview-1641966312
PR Review Comment: https://git.openjdk.org/jdk/pull/15875#discussion_r1335782224
PR Review Comment: https://git.openjdk.org/jdk/pull/15875#discussion_r1335783253
PR Review Comment: https://git.openjdk.org/jdk/pull/15875#discussion_r1335785236
PR Review Comment: https://git.openjdk.org/jdk/pull/15875#discussion_r1335786014
PR Review Comment: https://git.openjdk.org/jdk/pull/15875#discussion_r1335790095
PR Review Comment: https://git.openjdk.org/jdk/pull/15875#discussion_r1335809266
PR Review Comment: https://git.openjdk.org/jdk/pull/15875#discussion_r1335794251
PR Review Comment: https://git.openjdk.org/jdk/pull/15875#discussion_r1335795417
PR Review Comment: https://git.openjdk.org/jdk/pull/15875#discussion_r1335804959
PR Review Comment: https://git.openjdk.org/jdk/pull/15875#discussion_r1335812715
PR Review Comment: https://git.openjdk.org/jdk/pull/15875#discussion_r1335814806
PR Review Comment: https://git.openjdk.org/jdk/pull/15875#discussion_r1335835680
PR Review Comment: https://git.openjdk.org/jdk/pull/15875#discussion_r1335854527
