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.porva...@oracle.com>:
Hi Mario,
2012/4/21 Pavel Porvatov<pavel.porva...@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
/*
* 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");
}
}
