<Swing Dev> Force JPopup to be always heavyweight

Fri May 25 13:54:41 UTC 2012

I created one:

http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7171806

Should I wait that it becomes public or can i just push the "fix"
right away (same reviewer and repository of course)?

Cheers,
Mario

2012/5/25 Alexander Scherbatiy <alexandr.scherbatiy at oracle.com>:
> 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/
>>
>>
>>
>

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