Hi, Petr.
The fix looks good.
On 3/19/14 12:44 PM, Petr Pchelko 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.
--
Best regards, Sergey.
