<Swing Dev> Force JPopup to be always heavyweight

[Mario Torre]

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).

Anyway, I hope this version is good enough for you to go in.

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
>



-- 
pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF
Fingerprint: BA39 9666 94EC 8B73 27FA  FC7C 4086 63E3 80F2 40CF

IcedRobot: www.icedrobot.org
Proud GNU Classpath developer: http://www.classpath.org/
Read About us at: http://planet.classpath.org
OpenJDK: http://openjdk.java.net/projects/caciocavallo/

Please, support open standards:
http://endsoftpatents.org/