<Swing Dev> Force JPopup to be always heavyweight

Fri May 25 12:24:04 UTC 2012

Ops, I just noticed that the test was not in the changeset, I applied
the patch on a clean tree and apparently forgot to do hg add...

Should i push it with the same bug id? or do I need another id for that?

Cheers,
Mario

2012/5/25 Mario Torre <neugens.limasoftware at gmail.com>:
> Hi Pavel,
>
> Thanks,
>
> I've pushed to the awt gate.
>
> Cheers,
> Mario
>
> 2012/5/24 Pavel Porvatov <pavel.porvatov at oracle.com>:
>> 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
>>>>
>>>
>>>
>>
>
>
>
> --
> 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/

-- 
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/