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.
