Bug 42741 - Please review a new Search SPI
Please review a new Search SPI
Status: RESOLVED FIXED
Product: utilities
Classification: Unclassified
Component: Search
4.x
All All
: P2 (vote)
: 4.x
Assigned To: Marian Petras
issues@utilities
: API_REVIEW_FAST
Depends on:
Blocks: 39584 42540 44310 46165
  Show dependency treegraph
 
Reported: 2004-05-03 20:18 UTC by Marian Petras
Modified: 2004-08-30 19:17 UTC (History)
1 user (show)

See Also:
Issue Type: TASK
:


Attachments
diff for module OpenIDE-X (30.48 KB, application/zip)
2004-08-17 15:11 UTC, Marian Petras
Details
diffs for related modules (22.46 KB, patch)
2004-08-17 15:11 UTC, Marian Petras
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Marian Petras 2004-05-03 20:18:12 UTC
Please review a new Search SPI:

http://utilities.netbeans.org/nonav/doc/search/development/API-changes/searchinfo/SearchAPI.html

It allows custom nodes to publish instructions how
they should be searched. It adds one SPI interface
class and one support class implementing the
interface to the (stable) API defined in module
Openidex.
Comment 1 Marian Petras 2004-05-03 20:22:28 UTC
The change just adds one simple SPI class to module OpenIDE-X, i.e. it
has no impact on existing modules.
Comment 2 Jesse Glick 2004-05-03 20:23:38 UTC
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.
Comment 3 Tomas Pavek 2004-05-05 14:10:00 UTC
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).
Comment 4 Jaroslav Tulach 2004-05-05 16:12:04 UTC
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.
Comment 5 Jesse Glick 2004-05-19 15:56:09 UTC
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.
Comment 6 Jaroslav Tulach 2004-06-01 15:14:59 UTC
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. 
Comment 7 Marian Petras 2004-08-17 15:08:44 UTC
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.
Comment 8 Marian Petras 2004-08-17 15:11:11 UTC
Created attachment 16889 [details]
diff for module OpenIDE-X
Comment 9 Marian Petras 2004-08-17 15:11:56 UTC
Created attachment 16890 [details]
diffs for related modules
Comment 10 Marian Petras 2004-08-17 16:34:04 UTC
All the documents and tests have been provided - changing back to TASK.
Comment 11 Jesse Glick 2004-08-17 16:37:56 UTC
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.
Comment 12 Marian Petras 2004-08-19 09:55:35 UTC
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.
Comment 13 Marian Petras 2004-08-19 09:58:06 UTC
Correction of paragraph 1.:

   "yes and do not ask for subfolders"

should be replaced with

   "yes, and traverse subfolders without asking"
Comment 14 Jaroslav Tulach 2004-08-24 16:33:51 UTC
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.
Comment 15 Jesse Glick 2004-08-24 17:38:51 UTC
FWIW I don't agree with making SearchInfo a final class, seems quite
unnecessary, and would make some kinds of delegation needlessly difficult.
Comment 16 Jaroslav Tulach 2004-08-24 17:48:16 UTC
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. 
Comment 17 Marian Petras 2004-08-25 10:47:23 UTC
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);

    }
Comment 18 Miloslav Metelka 2004-08-25 11:33:44 UTC
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.
Comment 19 Marian Petras 2004-08-25 12:52:19 UTC
After a discussion with Mila Metelka (face to face), I agree that
interface SearchInfo may be transformed to final class.
Comment 20 Marian Petras 2004-08-27 19:56:20 UTC
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
Comment 21 Marian Petras 2004-08-30 19:17:23 UTC
Implemented in the trunk - committed to the CVS server.


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo