<AWT Dev> [9] Review Request: JDK-6463901 Either generify or deprecate sun.awt.EventListenerAggregate

[Anthony Petrov]

Hi Petr,

>  259         if (flavorListeners.isEmpty()) {
>  260             currentDataFlavors = getAvailableDataFlavorSet();
>
>  261         }
>  262         flavorListeners.add(listener);

Now that the flavorListeners is never null, I believe that the condition 
should check the value of currentDataFlavors, rather than rely on 
emptiness of the listeners collection: note that there's removeListener 
method, so the collection may become empty again, and we don't want to 
reinitialize the currentDataFlavors for the second time.

As for the Set vs. List issue, note that Set implementations generally 
do not guarantee a particular iteration order - it may change after 
adding/removing listeners. This is particularly true for hash-based 
collections. So if we use a HashSet, listeners added by other code could 
be executed in different order at different times which could confuse 
some applications (maybe?). Since we don't expect a large number of 
listeners to be added to the collection, I don't think that removing a 
listener would be a bottle-neck (and this is the only operation that 
actually benefits from using HashSet in this code). So personally, I'd 
choose a List instead. But I don't have a strong opinion on this.

--
best regards,
Anthony

On 3/18/2014 7:47 PM, Sergey Bylokhov wrote:
> On 3/18/14 7:44 PM, Petr Pchelko wrote:
>> Hello, Sergey.
>>
>> Thank you for the review.
>> The updated version is located here:
>> http://cr.openjdk.java.net/~pchelko/9/6463901/webrev.01/
>>
>> It addresses both of your comments.
> I am curious why Set and not a List(ArrayList)?
>>
>> With best regards. Petr.
>>
>> On 18.03.2014, at 19:06, Sergey Bylokhov <Sergey.Bylokhov at oracle.com>
>> wrote:
>>
>>> Hi, Petr.
>>>
>>> A few notes:
>>>
>>> 314         if (prevDataFlavors != null && currentDataFlavors != null
>>> 315                 && prevDataFlavors.equals(currentDataFlavors)) {
>>> 316             return;
>>> 317         }
>>>
>>> I suppose we should return in case both of them will be null?
>>> Objects.equals should be a little bit more readable here.
>>>
>>>
>>> 440                 flavorListeners.stream()
>>> 441                         .filter(Objects::nonNull)
>>> 442                         .forEach(listener ->
>>> SunToolkit.postEvent(appContext,
>>> 443                                 new PeerEvent(this,
>>> 444                                         () ->
>>> listener.flavorsChanged(new FlavorEvent(SunClipboard.this)),
>>> 445                                         PeerEvent.PRIORITY_EVENT)));
>>>
>>> here is a place where we can reformat it to be more readable.
>>>
>>> On 18.03.2014 18:34, Petr Pchelko wrote:
>>>> Hello, AWT Team.
>>>>
>>>> Please review the fix for the issue:
>>>> https://bugs.openjdk.java.net/browse/JDK-6463901
>>>> The fix is available at:
>>>> http://cr.openjdk.java.net/~pchelko/9/6463901/webrev/
>>>>
>>>> The bug states that we should deprecate or generify the
>>>> EventListenerAggregate class.
>>>> However it's an internal class in sun.awt package so we could remove
>>>> it.
>>>>
>>>> I've used grep on JDK source to verify that this class is not used
>>>> any more.
>>>> Clipboard regression, functional and JCK tests run fine.
>>>>
>>>> With best regards. Petr.
>>>>
>>>
>>> --
>>> Best regards, Sergey.
>>>
>
>