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

[Petr Pchelko]

Hello, Anthony, Sergey.

Thank you for the review.

> 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.
I'm now initializing it in the constructor, the same time as the list is initialized.

> 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.
I don't think the iteration order would have any effect on the applications. The only reason I'm choosing a Set is to avoid duplicated notifications in case 2 identical listeners were accidentally added. It's actually would be a bug in the client code, so I'm OK to replace the Set with the List if you think it would be better.

With best regards. Petr.

On 18.03.2014, at 23:07, Anthony Petrov <anthony.petrov at oracle.com> wrote:

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