<Swing Dev> Force JPopup to be always heavyweight

[Pavel Porvatov]

Hi Mario,

> Hi Pavel,
>
> I uploaded a new patch here:
>
> http://cr.openjdk.java.net/~neugens/6800513/webrev.02/
>
> I don't really understand why one should call internal private api
> (realSync) when a public API is there (Toolkit.sync), that *should* do
> the same (even if it obviously doesn't).
Why do you think they should do the same?
>
> Anyway, I hope this version is good enough for you to go in.
Now the test looks without functionality problems but there are some 
code style mistakes and unnecessary code. E.g. duplicate code in the 
main method, class field passing as method parameters (the getPopup 
method) etc.

To avoid time spending I modified your test a little bit (see attach) 
and approve the fix.

Regards, Pavel
> Please, let me know what you think,
> Mario
>
> 2012/5/4 Pavel Porvatov<pavel.porvatov at oracle.com>:
>> Hi Mario,
>>
>>> 2012/4/21 Pavel Porvatov<pavel.porvatov at oracle.com>:
>>>
>>> Hi Pavel,
>>>
>>>> About the test:
>>>> 1. Now is 2012 :)
>>> Ops...
>>>
>>>> 2. You must access to Swing components only from the EDT (see
>>>> clickOnComponent(final Component comp) and other methods)
>>> Not sure if I understand correctly, all the access is done in the EDT
>>> already, unless I became very blind!
>>>
>>> The tests are run from the EDT, only exception is checkPopup, which
>>> just read a value after the execution, and I think this should be
>>> safe.
>> Yes, I missed the fact that the clickOnComponent method invoked on EDT.
>> That's because you used robot.delay(50) in the method. There is no sense to
>> use sleep methods on the EDT therad: you just freeze any event handling....
>>
>>>> b.
>>>> loop
>>>>         final Map<String, Boolean>    tests = new HashMap<>();
>>>>         tests.put("javax.swing.PopupFactory$HeavyWeightPopup", false);
>>>>         tests.put("javax.swing.PopupFactory$LightWeightPopup", true);
>>>>
>>>>         for (final String test : tests.keySet()) {
>>>> can be replaced by two simple invocations
>>> Actually, this means duplicate more code or introduce another method,
>>> not sure if this makes the code cleaner, but I can do it if you prefer
>>> so.
>>>
>>>> c. NoSuchFieldException, SecurityException, IllegalArgumentException,
>>>> IllegalAccessException can be replaced by Exception
>>>> d.
>>>> robot.delay(50);
>>>> robot.mousePress(InputEvent.BUTTON1_MASK);
>>>> robot.delay(50);
>>>> Just use Robot#setAutoDelay
>>>>
>>>> etc.
>>>>
>>>> 5. latch must be volitile. After test rewriting I think this variable can
>>>> be
>>>> removed at all
>>>>
>>>> Note that tests should be readable and simplest as far as possible
>>> The reason why the test is so complex is that I wanted to throw the
>>> exact exception and don't mix the reflection related stuff with the
>>> real test exception, that also basically means I don't want to save
>>> the exception and rethrow it later on (I've seen this in some other
>>> tests), I rather prefer to make this obvious and not hidden, but of
>>> course the code gets longer, and everything is complicated by the EDT
>>> invocations.
>> In your case reflection exception is also test failing.
>>
>>> Also, I'm not particularly happy with the use of reflection to access
>>> the filed and check the class name, since we're testing against an
>>> implementation detail, but I don't know how else I should test that we
>>> create an heavy weight window (which is really also just an
>>> implementation detail that leaked through the code up to the user,
>>> nobody should ever care about heavy weight and lightweight imho), so
>>> if you have a smarter idea, I would be happy to change the code.
>> I'm also trying to avoid reflection in tests but I don't see another
>> solution here
>>
>>> I will try to refactor the code but I would like to not invest
>>> significant time in that, I'll send you a revised patch later
>>> (hopefully!)
>> Yes, and that's the reason to write first version of test without any
>> errors. The test shouldn't have errors, because if it fails (on some
>> platforms with very specific configuration) we have to fix it (therefore we
>> are trying to keep all tests as clear and short as possible)...
>>
>> Your test is still have problems. E.g. setVisible invocation doesn't
>> guarantee that right after method Frame becomes visible (platforms dependent
>> behaviour). You can take a look at good test examples in repository, e.g.
>> here
>> http://hg.openjdk.java.net/jdk8/awt/jdk/rev/dfa2ea47257d
>>
>> Regards, Pavel
>>
>
>

-------------- next part --------------
/*
 * Copyright 2012 Red Hat, Inc.  All Rights Reserved.
 * Copyright (c) 2012, Oracle and/or its affiliates. All rights reserved.
 * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
 *
 * This code is free software; you can redistribute it and/or modify it
 * under the terms of the GNU General Public License version 2 only, as
 * published by the Free Software Foundation.
 *
 * This code is distributed in the hope that it will be useful, but WITHOUT
 * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
 * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
 * version 2 for more details (a copy is included in the LICENSE file that
 * accompanied this code).
 *
 * You should have received a copy of the GNU General Public License version
 * 2 along with this work; if not, write to the Free Software Foundation,
 * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
 *
 * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
 * or visit www.oracle.com if you need additional information or have any
 * questions.
 */

/*
 * @test
 * @bug 6800513
 * @summary GTK-LaF renders menus incompletely
 * @author Mario Torre
 * @library ../../regtesthelpers/
 * @build Util
 * @run main bug6800513
 */

import sun.awt.SunToolkit;

import javax.swing.*;
import java.awt.*;
import java.awt.event.InputEvent;
import java.beans.PropertyChangeEvent;
import java.beans.PropertyChangeListener;
import java.lang.reflect.Field;
import java.util.concurrent.Callable;

public class bug6800513 {

    private static JPopupMenu popupMenu;
    private static JMenu menu;
    private static JFrame frame;

    public static void testFrame(final boolean defaultLightWeightPopupEnabled,
            String expectedPopupClass) throws Exception {
        SunToolkit toolkit = (SunToolkit) Toolkit.getDefaultToolkit();

        SwingUtilities.invokeAndWait(new Runnable() {
            public void run() {
                JPopupMenu.setDefaultLightWeightPopupEnabled(defaultLightWeightPopupEnabled);
                createAndShowUI();
            }
        });

        toolkit.realSync();

        clickOnMenu();

        toolkit.realSync();

        Field getPopup = JPopupMenu.class.getDeclaredField("popup");
        getPopup.setAccessible(true);
        Popup popup = (Popup) getPopup.get(popupMenu);

        if (popup == null) {
            throw new Exception("popup is null!");
        }

        String className = popup.getClass().getName();
        if (!className.equals(expectedPopupClass)) {
            throw new Exception("popup class is: " + className +
                    ", expected: " + expectedPopupClass);
        }

        SwingUtilities.invokeAndWait(new Runnable() {
            @Override
            public void run() {
                frame.dispose();
                popupMenu = null;
            }
        });

        toolkit.realSync();
    }


    public static void clickOnMenu() throws Exception {
        Rectangle bounds = Util.invokeOnEDT(new Callable<Rectangle>() {
            @Override
            public Rectangle call() throws Exception {
                return new Rectangle(menu.getLocationOnScreen(), menu.getSize());
            }
        });

        Robot robot = new Robot();
        robot.setAutoDelay(100);

        robot.mouseMove(bounds.x + bounds.width / 2, bounds.y + bounds.height / 2);

        robot.mousePress(InputEvent.BUTTON1_MASK);
        robot.mouseRelease(InputEvent.BUTTON1_MASK);
    }

    private static class PopupListener implements PropertyChangeListener {
        @Override
        public void propertyChange(PropertyChangeEvent evt) {
            if (evt.toString().contains("visible") && ((Boolean) evt.getNewValue() == true)) {
                popupMenu = (JPopupMenu) evt.getSource();
            }
        }
    }

    public static void createAndShowUI() {
        frame = new JFrame();

        JMenuBar menuBar = new JMenuBar();
        menu = new JMenu("Menu");

        menu.add(new JMenuItem("Menu Item #1"));
        menu.add(new JMenuItem("Menu Item #2"));
        menu.add(new JMenuItem("Menu Item #3"));

        menuBar.add(menu);

        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.setJMenuBar(menuBar);
        frame.setSize(500, 500);

        PopupListener listener = new PopupListener();
        menu.getPopupMenu().addPropertyChangeListener(listener);

        frame.setVisible(true);
    }

    public static void main(String[] args) throws Exception {
        testFrame(false, "javax.swing.PopupFactory$HeavyWeightPopup");

        testFrame(true, "javax.swing.PopupFactory$LightWeightPopup");
    }
}