<Swing Dev> Force JPopup to be always heavyweight

Wed May 30 13:08:52 UTC 2012

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