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.pet...@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.bylok...@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.
>>>>
>>
>>
