Hello,
Looks like I forgot to actually add a link to the new version. Here it is:
http://cr.openjdk.java.net/~pchelko/9/6463901/webrev.02/
With best regards. Petr.
On 19.03.2014, at 12:44, Petr Pchelko <petr.pche...@oracle.com> wrote:
> 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.
>>>>>
>>>
>>>
>
