<Swing Dev> Force JPopup to be always heavyweight
Pavel Porvatov
pavel.porvatov at oracle.com
Wed May 30 13:54:28 UTC 2012
Hi Mario,
> 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?
What do you mean? Don't change the name of the test and use comment like
the following:
7171806: Missing test for bug ID 6800513 fix
Reviewed-by: rupashka
Regards, Pavel
>
> 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/
>>>>>
>>>>>
>>>
>
>
More information about the swing-dev
mailing list