Created attachment 116056 [details]
The new QuickSearch API

Created attachment 116057 [details]
The new QuickSearch class for convenience

Created attachment 116058 [details]
Associated changes in OutlineView

Created attachment 116059 [details]
A rough prototype addition of quick search to Tools/Options dialog.

Please review component-based QuickSearch API.

A single diff of all proposed changes would be more convenient to review in general.


[JG01] Javadoc for attach should clarify that constraints is as in Component.add.


[JG02] What if attach is called twice with different constraints? Currently the second constraints is silently ignored. Should either update constraints, throw ISE, or consistently throw ISE if attach is called twice regardless of constraints.


[JG03] Javadoc for isAsynchronous mentions QuickSearchListener#showNextSelection(javax.swing.text.Position.Bias) twice.


[JG04] new RequestProcessor("Quick Search RP", 1) should rather be new RequestProcessor(QuickSearch.class).


[JG05] findMaxCommonSubstring is misleadingly named and documented - what you are looking for is a prefix, not a general substring.


[JG06] Use @StaticResource on ICON_* for safety.


[JG07] "called in AWT" is technically meaningless; you cannot call a method inside a toolkit. What you mean is that it is called inside the Abstract Window Toolkit's event queue dispatch thread, managed by java.awt.EventQueue, so "EQ' would be the more meaningful abbreviation.

Also you need not specify that something is called "in a RequestProcessor thread"; just that it is called in some background thread.


[JG08] Use of Position.Bias in this context is odd. It is normally used to define how a mark will move when text is inserted or removed. The "direction of the next search result" seems unrelated in every sense except that the values may be "forward" or "backward". Maybe just use a boolean?


[JG09] You cannot make openide.explorer depend on spi.quicksearch without introducing major new and unwanted transitive dependencies: openide.filesystems (for the weird search provider registration system, maybe too late to clean up now) and openide.windows (for focus handling in AbstractQuickSearchComboBar, not obvious how to replace). Is there a known use case for it?


[JG10] Stray System.out.println in OptionsPanel.

BTW do not reassign to apireviews, just CC it.

Thanks for your comments, Jesse.
[JG09] is a problem that can be solved only by some eager module I guess. I'm trying to get rid of dependency on openide.windows and will prepare move of ProviderModel into an eager module...

That sounds like the wrong approach; spi.quicksearch will not function properly for its existing usages at all without provider registrations, and it would be weird to have to add another module for that.

Perhaps QuickSearch can rather be defined in openide.awt and used from openide.explorer and options.api, leaving spi.quicksearch alone. QS anyway does not seem to use anything from the rest of spi.quicksearch other than the easily copied AnimationTimer, right?

Well, openide.awt depends on openide.filesystems as well. So in fact, I see that openide.explorer already have transitive dependency on openide.filesystems.
In that point of view it looks like it does not matter much if QuickSearch is in spi.quicksearch or openide.awt.
But maybe, since the spi.quicksearch is strictly service-oriented, the QuickSearch really belongs more into openide.awt as an UI utility.

Created attachment 116095 [details]
The new QuickSearch API in openide.awt module

Created attachment 116096 [details]
The new QuickSearch class for convenience

Created attachment 116097 [details]
Associated changes in OutlineView

[JG01]-[JG09] addressed in new attachments.
[JG10] Changes in options were prototype only to verify the quick search integration, it'd need more touch-ups before an integration.

Looks better.


[JG11] Check whether it is possible to avoid duplicating those icons - perhaps spi.quicksearch and openide.explorer can use the icons as defined in openide.awt?

Thanks.
[JG11] Yes, I had this in mind as well, after this is pushed to trunk, spi.quicksearch can reuse the icons from openide.awt. openide.explorer should not need them any more.

Thanks for the review, I'm going to integrate it later today.

Y01 It would be better if getQuickSearchListeners could be private.

Y02 setAsynchronous should in my opinion be specified at creation/attach time.

Y03 If setEnabled could be hidden in favor of detach, I'd be more glad.

Y04 You could use putClientProperty(QuickSearch.KEY) to avoid anyone else accessing QuickSearches registered by others. 

Y05 Is there a usecase when one would use more than one QuickSearchListener? If not, then again, it could be specified at attach time.

Y06 QuickSearchListener does not extend EventListener, e.g. it is not listener from point of view of JavaBean specification.

Y07 I don't understand the purpose of findMaxPrefix yet.

Thanks for the comments,
Y01: I see, but I'm just following the concept of Component and JComponent, which provide public access to listeners. I'd like to keep the flexibility there, e.g. for the case if QuickSearch is reused by debugger views.

Y02: Well this would hurt flexibility. The search criteria can be customized via actions in popup menu (setPopupMenu()) and it may be handy to be able to change the asynchronous behavior as needed.

Y03: O.K. it looks like currently setEnabled() is not really needed. detach() can be used to remove QuickSearch, but it could not be enabled later in case there is no access to the original component or to the setup logic. E.g. in OutlineView as soon as you call detach(), you can not enable the quick search again.

Y04: I'm not sure about the purpose, where do you propose to use the client property?

Y05: I was rather thinking about setQuickSearchListener() instead of add/remove. But again, I think that the current API is more flexible and changing this may limit future usage. E.g. in OutlineView, one may want to add it's own listener to the provided QuickSearch.

Y06: Good point, will make it extend the EventListener, thanks.

Y07: findMaxPrefix() is used by current quick search to do TAB-completion. To find the maximum prefix sub-string that can be auto-completed when TAB is pressed, without necessity to write the characters explicitly.

Usually I would say, that it is up to you. But this time, the code is from openide.explorer and is supposed to end up in openide.awt, and both places are very close to the area that I maintain. E.g. I'd rather have there code which will be joy to maintain than something overly flexible. 

Re. Y01 - I believe this is a flawed design. I don't think there is any benefit in just keeping consistency when someone does a mistake. Please make it package private and let's wait till somebody needs that (I sort of doubt that will be the case).

Re. Y02 (and sort of Y03) - OK, if you think so. Having this mutable state cannot hurt much, if there is real usecase for it.

Re. Y04 - I want people to prevent removing QuickSearch from components they have not added it to. Right now it is simple - just use anyJC.putClientProperty("org.openide.awt.QuickSearch", null). As soon as you create private static final Object KEY = new Object(), this will be prevented.

Y05 - You can always make the API more flexible in future. In contrast to Y01, having add/removeXYZListener is at least standard JavaBeans pattern. Keep it, if you wish. Btw. rather than setQuickSearchListener, the JavaBeans spec suggest to have void addQuickSearchListener(QuickSearchListener l) throws TooManyListenersException - but that is not pleasant to use, especially when it is clear there will always be only one listener. As I mentioned, specifying it in the factory seems to be the most smooth solution of the only known usecase.

Y07 - Additional point. Methods in listener usually return void (maybe this is prescribed by the JavaBeans specification). Having method String findMaxPrefix(String) in there turns the interface into some kind of controller, not a listener. There should be only one such controller it does not make sense to have more than one (althrough your solution chaining the results is interesting). Also I don't like the default static method to compute the prefix - it signals that for majority of usages, people don't need to override it method. 

What about: creating new interface with only the findMaxPrefix method and have two factory methods - one taking this interface, the other not and providing default implementation by calling the static method findMaxPrefix which can be made private.

Y01: When someone is adding a new QuickSearchListener, I think it's a good option to be able to remove the original listeners or not, or call the original ones from the newly added listener, depending on the particular use-case.
But if that sounds too wild, I'll make the getQuickSearchListeners() package private until it's requested by some use-case.

Y05: O.K. I'll keep add/removeQuickSearchListener.

Y07: I agree that findMaxPrefix() differs from other methods in the sense that it returns some value instead of performing search navigation. But I'm not sure it's worth to create a new interface for it. The static method is just a utility that is usually used by the implementation, it can not be used as an implementation itself, since it does not know all the possible values.
Look e.g. at TableQuickSearchSupport.findMaxPrefix() for an example of an implementation.
The quite inefficient original implementation of TAB-completion persuaded me that the static utility method is handy, but it needs to be used by the implementation logic.

Martin just told me that debugger will need to modify the default behavior of QuickSearch provided by Outline view to skip search over "Evaluating..." cells. Doing that with two QuickSearchListeners acting in an orchestration seems to fragile to me. I'd rather divide the responsibility between Outline view (enumerating & selecting cells) and debugger (knowing cells to search and how).

We don't know how to handle it optimally yet.

Created attachment 116357 [details]
A modified QuickSearch API in openide.awt module

I've simplified the QuickSearch class and decided to really restrict it to one listener - now called Callback. It'd be upon the QuickSearch API user to decide whether to provide some sort of access to the Callback logic.

Created attachment 116358 [details]
The new QuickSearch class for convenience

Created attachment 116359 [details]
Modified associated changes in OutlineView, QuickSearchTableFilter class introduced to allow filtering of quick search

OK, this is less misusable. Two more advices to tighten the design even more:

Y08 OutlineView returns QuickSearch. I don't think there is any need for that. Just expose setQuickSearchAllowed(boolean). Reasons in favor of such approach:
- it would be consistent with TreeView
- it would be easier to use in Matisse (is it possible to visually change property of getQuickSearch() at all?)
- it would lower the concepts one has to understand when using TreeView/OutlineView by leaving QuickSearch API as implementation detail.

Y09 asynchronous() should rather be a property that cannot change during existence of QuickSearch rather a method on a Callback interface (mentioned by Y02). Is your plan to return different value on odd/even calls? I doubt it would have any reasonable meaning. Together with Y08 this would imply addition of setQuickSearchAsynchronous(boolean) to TreeView and OutlineView.

One more implementation note: The QuickSearch is created in TreeView (and OutlineView) every time. Why? Sounds useless (when the default is to not use it). Why you don't wait till somebody enables it by calling the TreeView.setQSA and (re)create it then? Btw. such approach would eliminate the need for QuickSearch.setEnabled as Y03 suggested.

Created attachment 116426 [details]
A modified QuickSearch API in openide.awt module

Created attachment 116427 [details]
Modified associated changes in OutlineView

Created attachment 116428 [details]
The new QuickSearch class for convenience

I've modified the APIs according to the comments. My opinion is, that a public getQuickSearch() is more flexible towards future evolution, as new features that are added to QuickSearch would be available. But since there's no added value currently and to be consistend with other views, I've added the isQuickSearchAllowed() and setQuickSearchAllowed() method.

asynchronous is a parameter to attach() now, it's true that it's not expected that it's value would change. Although I can imagine this e.g. in OutlineView that the asynchronous behavior might be dependent on the set of visible columns. But a rare use-case can be addressed by a call to setQuickSearchTableFilter().

The QuickSearch is created in views, because it's enabled by default.

If there are no more remarks, I'd finish some remaining javadoc and push the change tomorrow.

Thanks, I don't have any other comments.

The API is pushed as changeset:   214561:5d1e835e35a9
http://hg.netbeans.org/main/rev/5d1e835e35a9

The icons are reused in changeset:   214562:9b2b7e8060c0
http://hg.netbeans.org/main/rev/9b2b7e8060c0

Integrated into 'main-golden', will be available in build *201203090400* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/5d1e835e35a9
User: mentlicher@netbeans.org
Log: #208794: QuickSearch API introduced. It's attached on explorer views, QuickSearchTableFilter interface added to control the quick search in OutlineView.