Sorry, hit Commit too early.

Yarda noticed that LookupsProxyTest.testProxyListener was failing (for
how long? not sure). Reason: the test first proxies to Lookup.EMPTY,
using Lookups.proxy, and EMPTY returns as its allItems
Collections.EMPTY_SET. Then changed delegate to an empty
InstanceContent, which returns a List of size 0. This caused a
LookupEvent to be fired, since the List was not equal to the empty set
(lists and sets are never equal).

Yarda patched this in openide/src/org/openide/util/Lookup.java 1.23,
to return EMPTY_LIST from Empty.allItems, mainly I guess to make this
test pass.

I have a planned patch (will attach) which reverts that and solves the
problem differently, by treating Set's as List's when comparing in
Lookups.proxy; since normally people are interested in taking the
Collection and getting an Iterator from it, which effectively means
List semantics. The patch returns Set from Lookup.EMPTY, more to
ensure that it works than anything else (it should not matter).

Another possible "fix" would be to argue that changing the result of
the Lookups.proxy's allItems from an (empty) Set to an (empty) List is
really a change that should fire a LookupEvent, therefore the *test*
was wrong.

Confusion stems from use of Collection where either List or Set would
have been more appropriate.

- Is order significant? Surely yes, since it is common to get an
iterator and use the first result you find; in fact some impls of
lookup(Class) do exactly that. So using a Set is  strange (unless it
is a SortedSet I suppose, or a LinkedHashSet).

- Are duplicates permitted, e.g. in Lookup.Item's from allItems, or
Object's from allInstances? Javadoc does not say one way or another.
InstanceContent appears to in fact produce duplicates if you add the
same (equals()) instance more than once, but I did not confirm this.

Created attachment 10995 [details]
Proposed patch

Created attachment 11014 [details]
Previous diff seems to allocate ArrayList if newPairs are EMPTY_SET, anyway would not this be simpler?

I guess you could special-case EMPTY_SET for newPairs too, though I
would not expect it to be common... suggested diff also allocates
Object[] arrays in the most common case, a change between two lists.

This issue has two parts: the code and Javadoc.

The first part I consider FIXED by Jesse. Yarda, if you think that
your patch is better and makes difference I suggest you to ask Petr
Nejedly to measure it and/or add his opinion here. To me it looks that
you are allocating array anyway.

The second part, the Javadoc, must be clarified.

The current usage is: lookup can contain duplicates;
Lookup.Result.allClasses() will ommit all duplicates and return only
unique class names; other methods - allItems and allInstances - will
return duplicates; most of our lookup usages expects ordered items.

Because of compatibility we cannot do much about the signatures. I had
to ask Yarda why allClasses returns Set. I think it should return the
same collection type as other two methods. He said it is how JINI works.

Also the fact that order is important in our lookup usage seems to me
unwanted. By using lookup I as client do not care about what I get, I
just want some instance. If I need to work with a concrete instance or
with instance user chosen I should rather not use lookup or use it but
store Lookup.Item.ID and next time retrieve that ID from the lookup.
From this point of view the order is not important and return value of
type Set would be more appropriate also for other two methods.

What should I do? I can add:
* "Result can contain duplicate items." to the Lookup.Result class Javadoc
* "All duplicate classes with be omitted." for Lookup.Result.allClasses
* "The return value type should be List instead of Collection, but it
is too late to change it. It is not recommended to rely on order of
items in the lookup. " for Lookup.Result.allInstances and allItems.

Will that help? If somebody implements lookup which returns Set it
will not be a problem. Just order can randomly changed, but it was
never guaranteed and I think we do not want to guaranteeded it in
future either.

"Also the fact that order is important in our lookup usage seems to me
unwanted."

Probably not. At least AbstractLookup is supposed to guarantee the
order. See for example:
http://www.netbeans.org/source/browse/openide/test/unit/src/org/openide/util/lookup/AbstractLookupTest.java.diff?r1=1.12&r2=1.13

"Result can contain duplicate items."
That is a fine clarification.

"All duplicate classes with be omitted."
This method is not well tested, sorry. But the intended behaviour is
to omit the duplicates. That is the reason it is a Set.

"The return value type should be List instead of Collection, but it
is too late to change it. It is not recommended to rely on order of
items in the lookup. "
I would say, that implementations of Lookup are encouraged to reflect
the order as for example AbstractLookup does.

OK, I slightly improved the Javadoc:

Checking in src/org/openide/util/Lookup.java
new revision: 1.25; previous revision: 1.24

IMO, the ideal state is completely opposite: all methods should return
Set what means no duplicates and undefined order. It is possible to
have an ordered implementation of Lookup but generally lookup design
does not guarantee order. However this is minor detail to me.

Currently Lookup does preserve order in various ways - from folders in
FolderLookup, from classpath in MetaInfServicesLookup. Whether this is
*good* or not is a different question. It's not a detail, I think; a
pretty basic question about the intent of the API.

I tend to agree that Set would have been better, leaving any ordering
semantics up to domain-specific techniques (e.g. adding priorities or
queriable named capabilities to the interfaces which are searched
for), or something like Registry which defines particular names and
locations for objects. I consider Lookup more like a discovery service
or replacement for Java's instanceof.

closed