RFR: JDK-8290469: Add new positioning options to PassFailJFrame test framework [v3]

[Harshitha Onkar]

On Fri, 22 Jul 2022 20:31:44 GMT, Phil Race <prr at openjdk.org> wrote:

>> Following is a proposed solution to push the latest position changes to the window manager.
>> 
>> Robot.waitForIdle() are added between setLocation() and the subsequent getLocation() calls. This ensures any pending location updates are pushed to the window manager before the new location of the frame is in turn used to position the testWindow.
>> 
>> In multiple test window cases ONLY the test instruction frame and the main/root test window are positioned using `positionTestWindow()`, the rest of the windows are positioned within EDT at test-level.
>> 
>> **Impact of the proposed fix:** 
>> With this change `positionTestWindow()` can no longer be called on EDT at test-level, hence any existing manual tests calling this method on EDT requires changes.
>> 
>> For example: In this particular test, the following line `PassFailJFrame.positionTestWindow(frame,PassFailJFrame.Position.HORIZONTAL)` should be called after `createAndShowGUI()`
>> 
>> https://github.com/openjdk/jdk/blob/d333df8ff59c6242171da1a6d02d05e6a8e65c9d/test/jdk/javax/swing/JComboBox/JComboBoxActionEvent.java#L75
>>  
>> 
>> public static void positionTestWindow(Window testWindow, Position position) throws AWTException {
>> 
>>         if (isEventDispatchThread()) {
>>             throw new Exception("positionTestWindow() should not be called on EDT");
>>         }
>> 
>>         robot = new Robot();
>>         Dimension screenSize = Toolkit.getDefaultToolkit().getScreenSize();
>> 
>>         if (position.equals(Position.HORIZONTAL)) {
>>             SwingUtilities.invokeLater(() -> {
>>                 int newX = ((screenSize.width / 2) - frame.getWidth());
>>                 frame.setLocation(newX, frame.getY());
>>            });
>>             // waits until the queue is flushed
>>             robot.waitForIdle();
>>             robot.delay(300);
>> 
>>             SwingUtilities.invokeLater(() -> testWindow.setLocation((frame.getX() +
>>                                              frame.getWidth() + 5), frame.getY()));
>> 
>>         } else if (position.equals(Position.VERTICAL)) {
>>             SwingUtilities.invokeLater(() -> {
>>                 int newY = ((screenSize.height / 2) - frame.getHeight());
>>                 frame.setLocation(frame.getX(), newY);
>>             });
>> 
>>             robot.waitForIdle();
>>             robot.delay(300);
>> 
>>             SwingUtilities.invokeLater(() -> testWindow.setLocation(frame.getX(),
>>                                             (frame.getY() + frame.getHeight() + 5)));
>> 
>>         } else if (position.equals(Position.TOP_LEFT_CORNER)) {
>>             SwingUtilities.invokeLater(() -> {
>>                 GraphicsConfiguration gc = GraphicsEnvironment.getLocalGraphicsEnvironment()
>>                         .getDefaultScreenDevice().getDefaultConfiguration();
>>                 Insets screenInsets = Toolkit.getDefaultToolkit().getScreenInsets(gc);
>>                 frame.setLocation(screenInsets.left, screenInsets.top);
>>             });
>> 
>>             robot.waitForIdle();
>>             robot.delay(300);
>> 
>>             SwingUtilities.invokeLater(() -> testWindow.setLocation((frame.getX() +
>>                    frame.getWidth() + 5), frame.getY()));
>>         }
>>     }
>
> We should discuss that impact. 
> This method is useful to simplify the logic of individual tests but is it now becoming awkward to use ?
> Or is it something that we should have required from the beginning  ?
> And why invokeLater and not invokeAndWait ? I'd have expected Later to not work for this case ..

The interleaving does seem to make the code more complex. And at test-level, it causes some confusion as few method need to called within EDT and few out of EDT. 

I wanted to check if there was a simpler, much clearer approach and evaluate possible options before proceeding.

I might have gone with invokeLater, since we are calling waitForIdle() later  and it waits until the queue is empty. But I think invokeAndWait would be a better option here.

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

PR: https://git.openjdk.org/jdk/pull/9525