<Swing Dev> Force JPopup to be always heavyweight

Wed May 30 13:17:45 UTC 2012

Hi Pavel, sure,

Should I use the
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7171806 that I
created as CR for the test or the same  CR 6800513?

Cheers,
Mario

2012/5/30 Pavel Porvatov <pavel.porvatov at oracle.com>:
> Hi Mario,
>
> I marked CR 6800513 as fixed. Could you please push the test (with me as a
> reviewer)?
>
> Thanks, Pavel
>
>> 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/