The change just adds one simple SPI class to module OpenIDE-X, i.e. it
has no impact on existing modules.

I would suggest that all occurrences of DataObject and
DataObject.Container be replaced with FileObject, to make it easier to
move away from Datasystems later.

As the whole Search API uses DataObjects, I think it is not that
important at this moment. We talked about this at DevRev and there was
no objection. IMHO it would not be easy to make this addition
independent on DataObject and work with the rest of the search API
(but I have not checked it closer).

Originally we thought that the openidex will survive till the end of
times without modifications. But as it seems to be used, we decided
that we need to turn it into real api. Which means we need to:

1. generate javadoc
2. write arch-*.xml questions document
3. maintain apichanges

Marian, as you are requesting a change there, please write initial
version of arch-*xml document. Provide general description and mention
things you have modified. 

javadoc target and apichanges should be easy. Description is available
here:
http://openide.netbeans.org/tutorial/api.html
I am not sure if you want to keep this as fasttrack or turn into real
review.

OK, if the API already refers to DataObject there is no harm in
continuing that.

FWIW I don't see any problems with the proposed changes otherwise.

Marian, you told me that the review is going to happen in middle of
May. You got the comments, now it is probably time to implement them. 

Please review again. There has been some changes made since the last
review. The change is compatible if related to NetBeans 3.6 and
incompatible if related to the previous review. However, modifications
for all standard NetBeans modules are ready so the change should not
make NetBeans not buildable.

Briefly:
 - interface SearchInfo remained unchanged
 - class SimpleSearchInfo is no longer public  (is package-private now)
 - added factory class SearchInfoFactory
     - replaces class SimpleSearchInfo
     - adds support for filtering acc. to Visibility/SharabilityQuery
 - added public class DelegatingSearchInfo

The target milestone is NetBeans 4.0.

The main reason for the change was to prevent bugs such as
   bug #44310 (Search should ignore nonsharable files)
   bug #46165 (Search should skip files which are hidden acc. to
               VisibilityQuery)
Factory class SearchInfoFactory also makes definition of SearchInfo
objects easier for some cases and does not introduce any new problems
I would know at the moment. It also allows easier modification of
implementation without any need for API change.

This API change requires code changes in standard NetBeans modules
java, projects and utilities. Change in module dependencies for
building requires also some small changes in nbbuild/build.xml.

Javadoc and architecture documentation is available for interface
SearchInfo and related classes - use "ant javadoc" and "ant arch". An
initial apichanges document is ready, too.

I am attaching two diffs - the first one contains just changes within
Openidex module, the other contains related changes in other modules.

Created attachment 16889 [details]
diff for module OpenIDE-X

Created attachment 16890 [details]
diffs for related modules

All the documents and tests have been provided - changing back to TASK.

Basic API looks OK. Possible problems:

1. The factory hardcodes a set of criteria to filter by. (Visibility,
etc.) This is probably necessary to keep the client API easy for
common cases. However it seems that the impl is rather complicated and
special-cased; generally I think you should be able to pass a filter
into the factory and get a SearchInfo back, where the filter can decide

- for a file, search it?

- for a folder, traverse it? if so, use the same filter, no filter, or
a different filter?

Such an API could (I think) handle all the special cases currently
handled plus more, without requiring the user to rewrite all of the
unrelated logic apparently needed to implement SearchInfo.

2. The "unit tests" for openidex are not that. They are functional
tests, perhaps. To make a unit test for this kind of thing, control
Lookup.getDefault to include your own implementations of
VisibilityQuery and SharabilityQuery. Then you do not need to have a
real project in your test data; just make a random tree of files and
set some of them visible or not, some sharable or not. This would
reduce the dependence of the unit test on the behavior of other
modules, as well as permitting you to test more corner cases that do
not (currently) arise in j2seproject.

3. Note that you can process arch.xml documents without needing a
special build script. Just name it "arch.xml" at the top of the module
(e.g. openidex/arch.xml) - will appear in the logical view of the
module, you will have a context menu item to process it, it will be
included in your Javadoc.

4. Is there any particular reason why you moved the search
registration from ProjectsRootNode to each project type? And why only
to java/j2seproject and not any of the other project types? If you are
going to require projects to specify their own search info (which
seems unnecessary to me, at least at the moment), it should be in the
project lookup, not the project root node.

5. Why createSearchInfoBySubnodes in J2SELogicalViewRootNode? IMHO
pressing Ctrl-F on a project should simply search all GENERIC source
groups in the project. What is displayed in the logical view is not
relevant.

6. Your openidex/test/build-unit.xml is trying too hard. You can
import nbbuild/templates/xtest-unit.xml and make it otherwise empty.
Everything is set up in project.properties.

ad 1. (implementation, fixed set of criteria)

I agree with the idea of filter API. I will modify the implementation
so that it uses it. But I am not sure the filter API should be public.
Why make it public if nobody needs it? Do you know of any use case
other than filtering by sharability and visibility?

Filter API implementations must answer the following questions:
 - for a file:  search it? (yes/no)    [as you suggested]
 - for a folder: traverse it? (yes/
                               no/
                               yes and do not ask for subfolders)


ad 2. (unit vs. functional tests)

It is a good idea, but I do not want to implement it immediately.


ad 3. (no need for a special script for processing arch.xml)

Thanks. I will modify it as you suggest.


ad 4. (SearchInfo registration - where to put it)

It is moved to java/j2seprojec so that each project type can define
how its root node should be searched. An alternative solution is that
a project's root node should search all sharable(?) and visible files
of in the project - see paragraph 5.


ad 5. (what should be searched if a project's root node is selected)

That's a question. My idea was to only search what you can see. I will
discuss this with people from HIE.


ad 6. (complexity of build-unit.xml)

OK, I will simplify it.

Correction of paragraph 1.:

   "yes and do not ask for subfolders"

should be replaced with

   "yes, and traverse subfolders without asking"

I would suggest usage of Enumeration instead of Iterator, at least the
code could reuse Enumerations and would be a bit simpler.

DelegatingSearchInfo is a bit unnecessary api that needs to be
maintained. I request it to be deleted and instead suggest following
code snipped to be suggested for usage in nodes:

public class MyNode extends Node {
     public MyNode() {
         this(new org.openide.util.lookup.InstanceContent ());
     }

     private MyNode(InstanceContent ic) {
         super(new AbstractLookup (ic));
         ic.add (SearchInfoFactory.createSearchInfoBySubnodes(this));
     }
 }

According to our guidelines I suggest SearchInfo to be final class and
 factory methods from SearchInfoFactory to be moved into it. I support
Jesse's request for callback interface that would answer the
"yes/no/all" question for folders.

Otherwise it would be nice if arch-usecases enumerated or pointed to
more examples of how this API can be used.

FWIW I don't agree with making SearchInfo a final class, seems quite
unnecessary, and would make some kinds of delegation needlessly difficult.

Agree vs. disagree is not important. The api should be compared to the
specified, written goals, not wishes.

None of the usecases talks about direct implementation of the
interface. If I am wrong and just overlooked it I am sorry. But if it
did it would be a poor design as it does not separate API and SPI. 

Enumeration vs. Iterator:

I do not see any advantage of Enumeration over Iterator, except the
absence of the remove() method (which is not usable in this case).

As regards simplicity of the implementation, I think you hint at
org.openide.util.Enumerations.queue(...). Unfortunatelly, this is not
usable in the implementation as it would not allow to take advantage
of answer "all" (from the yes/no/all triplet).


Usefulness of DelegatingSearchInfo:

I agree, the suggested solution is a good replacement.


SearchInfo - interface vs. final class:

I do not see any use case that would require direct implementation of
the interface at the moment, but on the other hand I do not see any
reason why not allow it. The org.openidex.search package already
contains SPI classes and interfaces (e.g. SearchType, SearchGroup,
SearchGroup.Factory) and SearchInfo would be just another SPI interface.

I do not understand how an interface can be replaced with a final class.


Callback interface:

I have already modified the implementation so that it uses the
callback interface. I will make it public and add a method to the
SearchInfo factory class which would accept a list of its implementations.

    public interface FileObjectFilter {

        public static final int DO_NOT_TRAVERSE = 0;

        public static final int TRAVERSE = 1;

        public static final int TRAVERSE_ALL_SUBFOLDERS = 2;

        public boolean searchFile(FileObject file);

        public int traverseFolder(FileObject folder);

    }

I agree with Jesse that we should not require SearchInfo to be final
class as this whole thing should be an SPI.
 Maybe you could add "support" subpackage for support classes to make
the SPI clearer but it's generally up to you - e.g. for
DataObjectSearchGroup, FileObjectSearchGroup, DelegatingSearchInfo -
that one will probably go away as Yarda suggested.

After a discussion with Mila Metelka (face to face), I agree that
interface SearchInfo may be transformed to final class.

Final decision (unless somebody objects against it):

- keep interface SearchInfo
- introduce interface FileObjectFilter (as described above)
- modify SearchInfoFactory methods so that it accepts an array
  of FileObjectFilters instead of two booleans for sharability and
  visibility
- provide FileObjectFilters for filtering by Visibility/SharabilityQuery
  as public static final fields of class SearchInfoFactory

Implemented in the trunk - committed to the CVS server.