This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
Created attachment 114996 [details] Patch - Search API The search API currently offers only limited range of possibilities to customize searching in platform applications. The proposed change introduces new ways to provide custom search algorithms and to invoke standard search tasks from custom code. Wiki page: http://wiki.netbeans.org/SearchApiChangesFor72
Created attachment 114997 [details] Patch - Utilities Search (implementation and usage of the API)
Created attachment 115000 [details] Patch - Search API
Please review.
Created attachment 115446 [details] Example Module - Search Provider
Please, could I have a few general questions? Do you find the Search Provider API useful and worth maintaining? I think it could used for specialized searching in some project types (e.g. Java, PHP, Issues), for searching in databases in platform applications, or for custom general searching algorithms (platform-specific, more options). Can I make old SearchType class (and related classes) deprecated? It is not used anywhere in the IDE, and probably no platform application has ever used it, as there is no documentation for it. Furthermore, the API's stability is "Under development". Any comments to new API classes and methods? Thank you.
Performance notes: I've created several FileMatchers and tried to use them for searching in core-main (files: *.java, *.xml, *.properies, *.xml, *.txt and some binary files (by accident); text: TextToFind, case insensitive). Results: 12 minutes - ASCII file-Mapped memory, ignoring encoding (The encoding itself is not probably big problem, but FileEncodingQuery can be very expensive. This matcher can be used for pre-checking). 12.5 minutes - File-mapped memory (only small files, multi-line) 13 minutes - Character sequence backed by file-mapped memory: (big files, multi-line) 14 minutes - InputStreamReader (big files, sinle-line only) 16 minutes Character sequence backed by input streams (big files, multi-line) (All the matchers above were used with searcher that traverses java.io.File objects.) 14.5 minutes - Matcher based on InputStreamReader used by searcher that traverses FileObjects (legacy FileInfo.filesToSearch iterator). There is a small overhead in creating FileObjects for files whose names do not match file-name pattern. If almost all files in the scope matches the pattern (this case), the difference is small, but when searching by file name, the overhead can be more significant. (Searchers using meta-data from VCSs will be, hopefully, able to work without java.io.File at all). Current plan is to create an "intelligent matcher" that will choose the most appropriate matcher for current search options and checked files.
Good that you decided to ask for full review. The size of the changes is really huge. I'd suggest to create a branch in Hg and setup a hudson job to compile, test and generate most recent javadoc. If you create the branch, I can help with the rest. Few observations follow: Y01 do we really have question-version="3.34"? The number is supposed to be the version of the questions, not your module. Y02 I suggest to create new module org.netbeans.api.search rather than deprecating half of org.openidex.search and creating new stuff in there. That will solve your dilemma of deprecating certain classes or not. Just deprecate whole module. Y03 There should be highlevel usecases in arch.xml describing what the API wants to support. Only then it can be evaluated if it is any good. Y04 SearchProvider should either be API or SPI. Currently it is both. Btw. the UI does not need to stay in utilities, it could imho be in the same module as the interfaces - e.g. there could be private protocols to talk between these two. Y05 Thanks for the performance measurements. Given the recent efforts of our CND team, it is important to concentrate on performance of FileObject based solution. The results seem to indicate reading the content is OK. If finding files matching a mask is the slow operation, please consider effect of FileObject.getChildren(String pattern) and its effective implementation in masterfs and remotefs.
I've created branch search_api_207436. I would be glad if you can help me with the hudson job. Thank you. Y01 - New arch.xml with correct version of questions was generated. Y02 - Created module org.netbeans.api.search. Y03 - Usecase added. Y04 - The code was split into several packages, some for API, some for SPI. I'm also planning to move code from Utilities to the new module.
Created attachment 115647 [details] Example Module - Search Provider
[JG01] After Y02, what is the compatibility story for the many modules (e.g. maven, projectui) using org.openidex.util? Will they continue to work as before, i.e. is there only one or a limited number of "clients" - utilities, utilities.project - which will interpret both SPIs? Your branch ought to upgrade at least a representative number of these to the new SPI, and if possible all of them in nb.org, but modules using the old SPI need to still work.
(In reply to comment #8) > I've created branch search_api_207436. I would be glad if you can help me with > the hudson job. http://deadlock.netbeans.org/hudson/job/prototypes-search_api_207436/ [JG02] Use org.netbeans.api.annotations.common where appropriate; e.g. SearchInfoDefinitionFactory.createSearchInfo should be declared something like: public static @NonNull SearchInfoDefinition createSearchInfo(@NonNull FileObject root, @NullAllowed SearchFilterDefinition[] filters) and if you feel the need to also do a runtime nullness check (here on 'root') use Parameters.notNull. [JG03] Numerous Javadoc warnings: .../api.search/src/org/netbeans/api/search/SearchInfoDefinitionFactory.java:102: warning - @param argument "folder" is not a parameter name. .../api.search/src/org/netbeans/api/search/SearchInfoDefinitionFactory.java:102: warning - @param argument "recursive" is not a parameter name. .../api.search/src/org/netbeans/api/search/SearchInfoDefinitionFactory.java:132: warning - @param argument "recursive" is not a parameter name. .../api.search/src/org/netbeans/api/search/SearchInfoDefinitionFactory.java:132: warning - Tag @see: reference not found: FileObjectFilter .../api.search/src/org/netbeans/api/search/SearchScopeOptions.java:197: warning - @param argument "useIgnoreList" is not a parameter name. .../api.search/src/org/netbeans/api/search/provider/SearchInfoUtils.java:124: warning - Tag @link: can't find objectsToSearch() in org.netbeans.api.search.provider.SearchInfo .../api.search/src/org/netbeans/api/search/provider/SearchListener.java:67: warning - @param argument "fileObject" is not a parameter name. .../api.search/src/org/netbeans/api/search/provider/SearchListener.java:76: warning - @param argument "directory" is not a parameter name. .../api.search/src/org/netbeans/api/search/provider/SearchListener.java:107: warning - @param argument "t" is not a parameter name. .../api.search/src/org/netbeans/api/search/ui/ComponentFactory.java:84: warning - @return tag has no arguments. .../api.search/src/org/netbeans/spi/search/SearchInfoDefinition.java:77: warning - Tag @link: can't find objectsToSearch() in org.netbeans.spi.search.SearchInfoDefinition .../api.search/src/org/netbeans/spi/search/SearchInfoDefinition.java:77: warning - Tag @link: reference not found: Files .../api.search/src/org/netbeans/spi/search/SearchInfoDefinition.java:77: warning - Tag @link: reference not found: Files#filesToSearch() .../api.search/src/org/netbeans/spi/search/SearchInfoDefinition.java:77: warning - Tag @see: reference not found: SearchInfoFactory .../api.search/src/org/netbeans/spi/search/provider/SearchProvider.java:76: warning - @return tag has no arguments. .../api.search/src/org/netbeans/spi/search/provider/TerminationFlag.java:57: warning - Tag @link: can't find getFilesToSearch( org.netbeans.api.search.SearchScopeOptions, org.netbeans.api.search.provider.SearchListener) in org.netbeans.api.search.provider.SearchInfo .../api.search/src/org/netbeans/spi/search/provider/TerminationFlag.java:57: warning - Tag @link: can't find iterateFilesToSearch( org.netbeans.api.search.SearchScopeOptions, org.netbeans.api.search.provider.SearchListener) in org.netbeans.api.search.provider.SearchInfo
[JG04] Continuing JG01, if o.openidex.util is to be deprecated then you should use [1] to make it that way. [1] http://bits.netbeans.org/dev/javadoc/org-openide-modules/org/openide/modules/doc-files/api.html#deprecation
Thank you very much for the hudson job and your comments. JG01 - I originally intended to use both API in utilites.project, but now I am also considering adding an API similar to FileEncodingQueryImplementation, say SearchInfoQueryImplementation, that can be implemented in both api.search and org.openidex.util. What would you prefer? JG02-JG04 - Will fix it as soon as possible. I'm currently moving search-related code from Utilities to Search in Projects API (Y04). I will probably need to add an SPI for registering search scopes (currently done via SearchScopeRegistry class in utilities and friend modules). Can I do it or should I implement some private protocol? I think public SPI can be useful.
(In reply to comment #13) > adding an API similar to FileEncodingQueryImplementation, say > SearchInfoQueryImplementation, that can be implemented in both api.search and > org.openidex.util Cannot really guess what such an SPI would look like. But the "query" terminology is used for SPIs which decouple parts of the app: one part has a question (e.g. editor: "what encoding does .../X.java use?"), and some other parts might be able to answer (e.g. project: "ISO-8859-2, because that is what project.properties says"). What you are talking about might be some kind of compatibility bridge (?) but it does not sound like a query.
(In reply to comment #14) > Cannot really guess what such an SPI would look like. I meant that there could be a query asking "what is the correct search info for this node" and answer like "it is this object because there is SearchInfoDefinition (or legacy SearchInfo) in the lookup of the node", but it is quite over-complicated. I think that I could simply move SearchScopeNodeSelection to utilites.project (where other search scopes are implemented) and support both old and new APIs only there. (I've moved the UI to the new module - core-main/rev/f9c82c58f1f2.)
JG03 - http://hg.netbeans.org/core-main/rev/3692ad7b7453 JG04 - http://hg.netbeans.org/core-main/rev/b51c0491531d
JG01 - I've moved all lookup-based implementations of SearchScopeDefinition to module utilities.project, so that they can support both new and old APIs. http://hg.netbeans.org/core-main/rev/9b05fc07db68 JG02 - http://hg.netbeans.org/core-main/rev/ed111177d833 (annotations added)
Some API changes to simplify working with default search filters: http://hg.netbeans.org/core-main/rev/ab53ccab470e I'm sorry for updating the API while review is in progress.
I’ve added support for api.search to module maven: http://hg.netbeans.org/core-main/rev/19120338b109 I’ve also removed dependency on searching from module projectui. http://hg.netbeans.org/core-main/rev/1163d15544fa I think it is no longer needed. Instead of using AlwaysSearchableSearchInfo, SubNodesSearchInfoDefinition.canSearch returns always true (prevention of bad performance and deadlocks). Can you please check the changes, Jesse?
I’ve removed import of deprecated class org.openidex.search.SearchInfo in module java.source on branch search_api_207436: http://hg.netbeans.org/core-main/rev/c3ddec93becb Jan, please let me know if you can find any problems in the change.
Module web.project was updated for api.search on branch search_api_207436: http://hg.netbeans.org/core-main/rev/84950fc5517f David, can you please check the modifications?
I’ve updated modules jumpto and java.project to use api.search instead of org.openidex.search: http://hg.netbeans.org/core-main/rev/3fe5fc85f7d9 http://hg.netbeans.org/core-main/rev/f5c44eb0abd8 Tomáš, could you check the modifications, please?
Tomáš, module php.project was modified as well. Could you please check the changes? http://hg.netbeans.org/core-main/rev/9d2d76c28ffc http://hg.netbeans.org/core-main/rev/0e3634fae119
Changes seem fine to me. Thanks.
Seems good to me.
(In reply to comment #21) > Module web.project was updated for api.search on branch search_api_207436: > http://hg.netbeans.org/core-main/rev/84950fc5517f > David, can you please check the modifications? Looks OK, thanks.
(In reply to comment #19) > http://hg.netbeans.org/core-main/rev/19120338b109 [JG05] Seems wrong that SearchInfoDefinition is in the SPI package while SearchInfoDefinitionFactory is in the API package. Surely they should be in the same package. (As to whether that is "API" or "SPI" it depends on what is the "caller" and what the "service" in this case.) [JG06] When the DataFolder (or FileObject) is in the node's lookup, does it really need to call SearchInfoDefinitionFactory.createSearchInfo and add that too? Seems like the search client should somehow consider it implied that if you select a node somehow representing a folder, "search" means "search (recursively) in this folder". Of course that would be less explicit, but it would reduce the need for deps on the API in various project types. > http://hg.netbeans.org/core-main/rev/1163d15544fa > > I think it is no longer needed. Instead of using AlwaysSearchableSearchInfo, > SubNodesSearchInfoDefinition.canSearch returns always true (prevention of bad > performance and deadlocks). OK, I guess you looked at bug #48685 and are dealing with those potential problems somehow.
Thank you for your comments. JG05 - Fixed: http://hg.netbeans.org/core-main/rev/b576783c57da (class moved) JG06 - It works as you say. If there is no need to change default behavior (search recursively under DataObject that is in lookup of node using default filters), neither SearchInfoDefinition nor SubTreeSearchOptions should be registered. SearchInfoDefinitionFactory.createSearchInfo is indeed used to create delegates for SearchInfo (API class) objects, but only internally, the objects are not registered anywhere. SearchInfo objects are usually created by search scopes. Most important search scopes are defined in module utilities.project. They can create SearchInfos for SearchInfoDefinitions, SubTreeSearchOptions and org.openidex.search.SearchInfo registered in lookups of nodes or projects. They can also create appropriate default SearchInfo objects. The main reason for having separate module is dependency on Project API and org.openidex.util. Existence of FileObject in lookups is not checked. Do you think it should be?
(In reply to comment #28) > JG06 - It works as you say. If there is no need to change default behavior > (search recursively under DataObject that is in lookup of node using default > filters), neither SearchInfoDefinition nor SubTreeSearchOptions should be > registered. OK... so then why does e.g. the maven module need to add a SearchInfoDefinition to the lookup of its logical view node at all, when it already has a DataFolder in that lookup? If what you say is true, then the maven -> api.search dep can simply be deleted, and same for most other project types. > Existence of FileObject in lookups is not checked. Do you think it should be? [JG07] Yes, in 7.1 (bug #199391) we moved to preferring FileObject to DataObject in node lookups, since constructing the DataObject can be more expensive. For compatibility reasons you need to check for both, but check for FileObject first. (jtulach please correct me if I am getting this wrong.)
You are right, dependency on api.search in maven is unnecessary, thank you. http://hg.netbeans.org/core-main/rev/3500ff580a62 It is still useful in some modules, e.g. searching in java packages should not be recursive, so explicit search info has to be registered. I've noticed that when searching in Configuration Files in a web project, the whole project folder is searched recursively. Fixed here: http://hg.netbeans.org/core-main/rev/d15a5f45d958 JG07 - Fixed (http://hg.netbeans.org/core-main/rev/ca10065512bc)
(In reply to comment #30) > dependency on api.search in maven is unnecessary OK, thanks for clearing that up. I think there were other cases (project types?) where you have this unnecessary call. > It is still useful in some modules, e.g. searching in java packages should not > be recursive, so explicit search info has to be registered. Right, this makes sense. > I've noticed that when searching in Configuration Files in a web project, the > whole project folder is searched recursively. May want a similar search info in e.g. "Important Files" in apisupport.ant.
> I think there were other cases (project types?) where you have this unnecessary call. I’ve searched for all uses of new search API/SPI in netbeans modules: java.project: - non-recursive search in netbeans packages - searchable package node jumpto: - uses only default search filters php.project: - needs a custom search filter, so search info definition is registered explicitly utilities.project: - registers search scopes for projects (main, open) and selection web.project: - custom search info for Configuration Files Original org.openidex.search api/spi is used in these modules: cnd.makeproject: - custom non-trivial implementation of SearchInfo is used - I would rather ask the module owner to use the new API, as good knowledge of project model is needed editor: - editor find bar and find in projects dialog share SearchHistory object - Milutin has modified some code in trunk, so I will update it after branches are merged utilities.project - backwards compatibility It seems that there are no longer any unnecessary dependencies on Search API. > May want a similar search info in e.g. "Important Files" in apisupport.ant. I’ve registered explicit SearchInfoDefinition in lookup of ImportantFilesNode: http://hg.netbeans.org/core-main/rev/371b937e76a0
(In reply to comment #32) > I’ve registered explicit SearchInfoDefinition in lookup of ImportantFilesNode: > http://hg.netbeans.org/core-main/rev/371b937e76a0 [JG08] The trick with InstanceContent is awkward. In this case at least it would be much easier if createSearchInfoBySubnodes took a Children rather than a Node; after all, the only thing SubnodesSearchInfoDefinition does with the Node is call getChildren on it! That would reduce the usage code to ImportantFilesNode(Project project, Children ch) { super(ch, Lookups.fixed(project, SearchInfoDefinitionFactory.createSearchInfoBySubnodes(ch))); } Check other call sites (*) and consider such a change. For ConfFilesNode and PackageRootNode it seems you would still need a second private ctor, to use the Children twice, but at least you would not need to deal with AbstractLookup/InstanceContent, which seems more intuitive. For example: PackageRootNode(SourceGroup group) { this(group, new PackageViewChildren(group)); } private PackageRootNode(SourceGroup group, Children ch) { super(ch, new ProxyLookup(createLookup(group), Lookups.singleton(SearchInfoDefinitionFactory.createSearchInfoBySubnodes(ch)))); } Of course the description in arch.xml would need to be changed too. BTW this is a poor place to put such details of API usage; do not make users hunt around in the module-wide architecture description, which is not even visible from the code editor. The Javadoc for createSearchInfoBySubnodes should include a code sample (with CSS class="nonnormative"). arch.xml can enumerate high-level use cases for people curious what the purpose of the module is, but should refer to Javadoc for all details (at the method, class, or package level as appropriate). (*) hg grep -r 'branch(search_api_207436)' SearchInfoDefinitionFactory.createSearchInfoBySubnodes
By the way, I have _not_ made any attempt to review or even look at the full API; all my comments to date are just in reaction to specific usages in modules I know about, mainly project types.
JG08 - http://hg.netbeans.org/core-main/rev/1107bad543af Great idea, thank you. Method signature changed, example code moved to javadoc.
Created attachment 116089 [details] Diff of public packages
JG08 (2) - http://hg.netbeans.org/core-main/rev/2f7eec911ca2 Other code samples moved from arch.xml to javadoc.
Found some time to look over some of the API generally, by no means all of it. Hard to evaluate a lot of the API classes since they do not seem to be used anywhere, as if the implementation is incomplete. This is dangerous as it means that there may be major design flaws that will only appear the first time someone tries to use some subset of the API. The example Unix search provider seems reasonable, though it is missing some likely pieces such as a button to reopen the search control window, and it would probably be improved by SearchResultsDisplayer.createDefault - which is unimplemented. I made some minor impl changes in the branch. [JG09] SearchControl should follow some singleton pattern - have a private ctor and either add a getDefault() method, or make all methods static. [JG11] SearchProvider.Presenter.addUsabilityChangeListener is nonstandard; should be addChangeListener and have matching removeChangeListener. Can also consider making these methods final and adding protected final void fireChange(). [JG12] What is AlternativeProvider for? Seems to be broken dummy impl. (E.g. returns null illegally from composeSearch.) And why is BasicSearchProvider not registered? [JG13] Searching for something in my home dir, got a bunch of: java.nio.charset.MalformedInputException: Input length = 1 at java.nio.charset.CoderResult.throwException(CoderResult.java:260) at sun.nio.cs.StreamDecoder.implRead(StreamDecoder.java:319) at sun.nio.cs.StreamDecoder.read(StreamDecoder.java:158) at java.io.InputStreamReader.read(InputStreamReader.java:167) at java.io.BufferedReader.fill(BufferedReader.java:136) at java.io.BufferedReader.read(BufferedReader.java:157) at org.netbeans.modules.search.matcher.LineReader.readNext(LineReader.java:93) at org.netbeans.modules.search.matcher.SingleLineStreamMatcher.getTextDetailsSL(SingleLineStreamMatcher.java:120) [catch] at org.netbeans.modules.search.matcher.SingleLineStreamMatcher.checkMeasuredInternal(SingleLineStreamMatcher.java:90) at org.netbeans.modules.search.matcher.AbstractMatcher.check(AbstractMatcher.java:61) at org.netbeans.modules.search.matcher.DefaultMatcher.checkMeasuredInternal(DefaultMatcher.java:109) at org.netbeans.modules.search.matcher.AbstractMatcher.check(AbstractMatcher.java:61) at org.netbeans.modules.search.BasicComposition.start(BasicComposition.java:96) at org.netbeans.modules.search.SearchTask.run(SearchTask.java:103) Also when not searching for anything: java.lang.NullPointerException at org.netbeans.modules.search.BasicComposition.getRootFiles(BasicComposition.java:128) at org.netbeans.modules.search.ResultDisplayer.<init>(ResultDisplayer.java:70) at org.netbeans.modules.search.BasicComposition.getSearchResultsDisplayer(BasicComposition.java:116) at org.netbeans.modules.search.SearchTask.getDisplayer(SearchTask.java:182) at org.netbeans.modules.search.Manager.scheduleSearchTask(Manager.java:143) at org.netbeans.modules.search.SearchPanel.search(SearchPanel.java:271) at org.netbeans.modules.search.SearchPanel.actionPerformed(SearchPanel.java:259) at org.netbeans.core.windows.services.NbPresenter$ButtonListener.actionPerformed(NbPresenter.java:1340)
JG09 - SearchControl has static methods now http://hg.netbeans.org/core-main/rev/e8c1566b19dd JG11 - Methods for working with change listeners fixed http://hg.netbeans.org/core-main/rev/314f9b42a948 JG12 - Dummy provider removed http://hg.netbeans.org/core-main/rev/517d4ede23e4 BasicSearchProvider registered normally http://hg.netbeans.org/core-main/rev/e23b75b43bd2 JG13 - Exceptions caught http://hg.netbeans.org/core-main/rev/29d85ae4861f (List of exceptions will be shown in the UI) Browse scope implemented (hope it is the cause of problem) http://hg.netbeans.org/core-main/rev/db91d076961c > a button to reopen the search control window I originally did not plan to have this button in the default panel, but surely it will be useful. I'll finish the missing parts ASAP. Many thanks for your comments and fixes.
(In reply to comment #39) > JG11 - Methods for working with change listeners fixed > http://hg.netbeans.org/core-main/rev/314f9b42a948 Use ChangeSupport to simplify code. > JG13 - Exceptions caught > Browse scope implemented (hope it is the cause of problem) > http://hg.netbeans.org/core-main/rev/db91d076961c Maybe for the NPE; I guess the MalformedInputException was something else. >> a button to reopen the search control window > > I originally did not plan to have this button in the default panel The standard panel has a "Modify Criteria..." button in trunk, so it would be a usability regression if this were omitted, right?
(In reply to comment
(In reply to comment #40) (Sorry for previous post - an issue with login.) > Use ChangeSupport to simplify code. http://hg.netbeans.org/core-main/rev/2f638d1077db > The standard panel has a "Modify Criteria..." button in trunk, so it would be a > usability regression if this were omitted, right? I mean I did not plan to add that button to the default (simple) displayer returned by SearchResultsDisplayer.createDefault (displayer for standard search is not created by this method). But I agree it should be there for all providers, so I've extended the API a bit to allow it. Missing parts implemented: - SearchControl http://hg.netbeans.org/core-main/rev/eeaba55dc469 - SearchResultsDisplayer.createDefault http://hg.netbeans.org/core-main/rev/6034c47bc962 Example: http://wiki.netbeans.org/HowToUseSearchAPI#Provide_custom_search
(In reply to comment #42) > - SearchControl > http://hg.netbeans.org/core-main/rev/eeaba55dc469 [JG14] Any API taking a Class<? extends Something> is immediate cause for suspicion (normally candidates for replacement with a factory of some kind), and the Javadoc in this case does not really explain what the parameter is for. Can sort of guess by reading source code but not quite. Similarly in SearchResultsDisplayer.createDefault. > - SearchResultsDisplayer.createDefault > http://hg.netbeans.org/core-main/rev/6034c47bc962 Typos "righg", "addMathingObject" > Example: http://wiki.netbeans.org/HowToUseSearchAPI#Provide_custom_search [JG15] Consider replacing TerminationFlag with AtomicBoolean - might simplify code.
JG14 - http://hg.netbeans.org/core-main/rev/b2e8402340d4 Specifying class of search provider is not needed any more, as reference to provider is stored in presenter. JG15 - http://hg.netbeans.org/core-main/rev/cb1a91ab5034 TerminationFlag replaced with AtomicBoolean > Typos "righg", "addMathingObject" Thanks, fixed: http://hg.netbeans.org/core-main/rev/34396ae5176a
(In reply to comment #44) > JG14 - http://hg.netbeans.org/core-main/rev/b2e8402340d4 Just beware that Lookup.getDefault().lookup(BasicSearchProvider.class) will not work generally, but may happen to work if a call to Lookup.getDefault().lookupAll(SearchProvider.class) has already returned an instance of BasicSearchProvider. You can make this safer by using @ServiceProviders on BasicSearchProvider and registering it under both SearchProvider.class and BasicSearchProvider.class. (Cleaner would be if @ServiceProvider accept a static final field, so that you could easily refer to the singleton from other code, but for ServiceLoader compatibility it currently only accepts classes with no-arg ctors.) [JG16] Might be nicer if SearchInfo.getFilesToSearch returned Iterable rather than Iterator, so that you could use a JDK 5 for-loop: for (FileObject primaryFile : searchInfo.getFilesToSearch(sso, lstnr, tf)) {...}
By the way you really need to synch your branch with trunk [1] and resolve merge conflicts such as with 6469bf992654. When developing a complex branch like this it is wise to synch with trunk frequently, but the current divergence point is nearly a month ago. [1] wiki.netbeans.org/HgHowTos#Using_a_named_branch
(In reply to comment #45) >You can make this safer by using @ServiceProviders Thank you, fixed: http://hg.netbeans.org/core-main/rev/9fad9186baff JG16 - There already is SearchInfo.iterateFilesToSearch, but I guess name of the method is not well chosen. I could change the name, or remove it and modify getFilesToSearch to return Iterable. What would you prefer?
JG16 cont'd - I see now that there is iterateFilesToSearch as well. This is just confusing. Better to have one method returning Iterable. I am playing with writing a 'hg locate ... | xargs egrep ...' provider and reporting my findings: [JG17] It is a serious limitation that SearchProvider.isEnabled is given no context, such as the selection when Ctrl-F was pressed. I might want to offer a provider which only makes sense when searching over certain kinds of files - e.g. sources using a certain version control system, or on a remote server, or inside JARs, etc. I can return false from Presenter.isUsable after the fact, but then the tab has already been added to the search dialog. Of course if the presenter offers a free choice of scopes, then any initial context would be just a suggestion, so the provider should always be enabled if the current platform supports it. But for some providers it makes more sense to not offer a customizable scope, only operate on the selection defined before the dialog was opened. [JG18] The ChangeListener event set on FileNameComboBox and similar could perhaps be replaced with Component's built-in PropertyChangeListener, with defined PROP_ETC constants. [JG19] ComponentFactory is awkward to use from the GUI builder (bug #209061). Perhaps this utility could be redesigned so that it separates the view from the controller. For example, make ScopeComboBox extend Object and change the factory to work something like: public static ScopeComboBox createScopeComboBox(JComboBox component); so that the form would be responsible for actually creating the combo box, and the component factory would just adjust its properties such as model. This would be more pleasant since the call to ComponentFactory could happen outside the guarded block, assigning the result to a separate final variable, on which code completion would give only relevant methods. (In such a case JG18 would probably be irrelevant.) [JG20] It is awkward that Presenter.getForm is documented to be lazily initialized. Otherwise you could make the form be a top-level class and make the Presenter an inner class of it, simplifying a bunch of code. In fact the current impl of SearchPanel.init loads all the presenters and calls getForm on each of them eagerly, contradicting this advice! Nor can it avoid doing so with the current SPI, since JTabbedPane.add(Component) relies on Component.name for the tab title. You could define a String getTitle() in Presenter to let the tabbed pane be lazy about loading the components. But I think it would be more natural to declare that Presenter is permitted to eagerly create its associated form, but the Presenter will only be created lazily if and when the associated tab is clicked. That means that getTitle() should be a method in SearchProvider.
[JG21] Much of ScopeSettingsPanel is undocumented. It is not clear what e.g. the searchInGenerated property means. [JG22] It is unclear what a Presenter should do besides returning false from isUsable when there is some problem. Where should the details be displayed? Should it just include a JLabel somewhere in its form with a message? (Offering a NotificationLineSupport is one option for better integration with standard dialog UI.) [JG23] No way (that I can see) to associate a help ID with a Presenter. [JG24] The "Browse..." entry in the scope combo (in the standard presenter) does nothing when selected. [JG25] Some components coming from ComponentFactory do not seem to work at all. While the scope combo appears to work as it does in the standard presenter (incl. JG24), the filename combo is simply empty and cannot be edited, and the scope panel is also empty.
[JG26] "Modify criteria..." loses the original scope of the search.
>By the way you really need to synch your branch with trunk http://hg.netbeans.org/core-main/rev/7abb03ad3c12 >Better to have one method returning Iterable I’ll change it. > that SearchProvider.isEnabled is given no context I’m sorry, I’m not sure what the context should look like. TopComponentRegistry or actionsGlobalContext is not sufficient here? The isEnabled method is called every time the search dialog is opened. I’m afraid that it could be confusing if set of enabled providers depend on the global context at the time of opening. Mainly because the dialog is non-modal and global context can be changed. We could: - enable/disable tabs when global context change (probably complicated, maybe still confusing) - show tabs of all providers, use notification line to describe why the presenter is not currently usable (e.g. No versioned folder selected - users now they should select a versioned node, no need to open the dialog again) What do you think? > public static ScopeComboBox createScopeComboBox(JComboBox component) I really like this idea, will try to imlement it. > It is awkward that Presenter.getForm is documented to be lazily initialized You are right, it has no sense in this case. But it is useful if you pass a custom presenter to SearchResultsDisplayer.createDefault (it would be shown only if you click the “Modfy criteria” button). I think both Presenter and its form could be created lazily. Can it be? I will fix other bugs as well.
JG16 - http://hg.netbeans.org/core-main/rev/32158e1343bd SearchInfo.getFilesToSearch returns Iterable JG19 - http://hg.netbeans.org/core-main/rev/bf7d7a680d27 ComponentFactory separates the view from the controller I'll also consider renaming some classes and methods JG20 - http://hg.netbeans.org/core-main/rev/b0dff281500f Presenters created lazily JG21 - Javadoc added (included in JG19) JG22 - http://hg.netbeans.org/core-main/rev/913ccc4a02aa NotificationLineSupport added JG23 - http://hg.netbeans.org/core-main/rev/757e0908bd35 Help ID association JG24 - http://hg.netbeans.org/core-main/rev/5ba793150f5a Browse... scope opens file chooser JG26 - http://hg.netbeans.org/core-main/rev/1eea17ba7a7a Search scope for storing last SearchInfo
JG25 - http://hg.netbeans.org/core-main/rev/626093e925e6 Components coming from ComponentFactory fixed >I'll also consider renaming some classes and methods http://hg.netbeans.org/core-main/rev/628de8073898 Please check new names. Improved reusability of combo box for selection of search scope, clean method not needed any more: http://hg.netbeans.org/core-main/rev/b9fc06636c37
Please, do you have any further ideas or comments? When the API and UI is usable, can I merge it to trunk, or should I continue working on the branch?
(In reply to comment #51) > it could be confusing if set of enabled providers depend on the > global context at the time of opening. Mainly because the dialog is non-modal > and global context can be changed. I see - this would not work then. > e.g. No versioned folder selected - users [k]now they should select > a versioned node, no need to open the dialog again Awkward but probably OK. >> It is awkward that Presenter.getForm is documented to be lazily initialized > > You are right, it has no sense in this case. But it is useful if you pass a > custom presenter to SearchResultsDisplayer.createDefault (it would be shown > only if you click the “Modfy criteria” button). I do not think so, since you would have already created the form in this case. Anyway I think core-main #82d5d70846bc shows that the result is a lot simpler. BTW getForm Javadoc is still the same. 1eea17ba7a7a does not seem to be working. Just "Browse..." appears in the scope, and the filechooser pops up.
[JG27] If your SearchComposition.start impl says listener.searchStarted(); try { .... } finally { listener.searchFinished(); } (as might seem reasonable from the Javadoc), extra "Searching..." tasks will keep appearing and never close! Either remove these methods from SearchListener, or make GraphicalSearchListener's impl idempotent, etc.
(In reply to comment #54) > When the API and UI is usable, can I merge it to trunk, or should I continue > working on the branch? As far as I am concerned you can merge to trunk whenever you feel comfortable. Remember to "close" the branch first: http://wiki.netbeans.org/HgHowTos#Using_a_named_branch
> ...getForm Javadoc is still the same. http://hg.netbeans.org/core-main/rev/434fb0a2a451 I agree. Lazy initialization is useful only if you create a new presenter for results panel (to be shown with "Modify criteria" button), which is a corner-case. > 1eea17ba7a7a does not seem to be working. Just "Browse..." appears in the > scope, and the filechooser pops up. When you manually select "Browse..." scope from the combo-box, a file chooser should pop up and new scope "Browse... [selectedFileName]" should appear (and be selected). When the "Browse..." button is not selected manually, file chooser should pop up when the search is started. Does it not work this way for you, or do you think that the intuitiveness should be improved? JG27 - http://hg.netbeans.org/core-main/rev/cbe3869ace03 Confusing methods removed from SearchListener.
(In reply to comment #58) >> Just "Browse..." appears in the scope > > When you manually select "Browse..." scope from the combo-box That is not the problem. The problem is that the actual scope with which the search was started does not appear in the combo; the combo contains just one item, "Browse...", which is selected; and as soon as the presenter is redisplayed after Modify Criteria..., the file chooser pops up on top of it. I expect the redisplayed presenter to contain the previous search scope, selected, with "Browse..." as an unselected option.
> I expect the redisplayed presenter to contain the previous search scope, > selected, with "Browse..." as an unselected option. I understand now. Thank you for explanation. This should help: http://hg.netbeans.org/core-main/rev/2c8f077d1f1b Another option is to create a new Presenter with additional scope that stores the last search info (like BasicComposition.LastScopeDefinition). What about making LastScopeDefinition public?
(In reply to comment #60) > Another option is to create a new Presenter with additional scope that stores > the last search info (like BasicComposition.LastScopeDefinition). Sorry, do not understand this well enough to comment.
> Sorry, do not understand this well enough to comment. I am sorry. I meant the you can pass a new instance of Presenter to SearchResultsDisplayer.createDefault. It will be shown if "Modify Criteria" is clicked. The new presenter can contain a scope combo box with an additional search scope (new ScopeController(combo, scopeId, extraSearchScopes...)). And the additional search scope can hold the last used search info. SearchScopeDefinition that can hold last search info is implemented as BasicComposition.LastScopeDefinition, but it is not public. I think most users will simply reuse the current presenter and publishing LastScopeDefinition is not very useful.
The branch is merged so I guess this can be closed as FIXED.
I've added a new method SearchResultsDisplayer.setInfoNode(Node). Can you please check it? I'm sorry for changing API on the trunk, this should be the last change. http://hg.netbeans.org/core-main/rev/a090b55029d5
setInfoNode - seems OK; no strong opinion. Since this issue is implemented in trunk it should be marked FIXED. Blocked issues can remain open unless and until addressed.
Created attachment 117164 [details] Example Module - Search Provider
> Since this issue is implemented in trunk it should be marked FIXED. Thank you very much for all your help.
Created attachment 118467 [details] [FYA] what a SearchType was in 3.0
Possible search provider impl for someone to play with: http://en.wikipedia.org/wiki/Windows_Search