<Swing Dev> Force JPopup to be always heavyweight

Fri May 25 13:45:12 UTC 2012

On 5/25/2012 4:24 PM, Mario Torre wrote:
> 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?

     I guess that it is not possible to make one more commit with the 
same bug id.

     Just create an issue that the test should be added to the repository.

     Thanks,
     Alexandr.

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