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.
Editor find and replace dialog and search over files in explorer should have one and shareable history. Editor module should be notified about last selected search expression to highlight(in yellow) the matched patterns. Typical use case: Editor registers a listener to listen on lastSelected SearchPattern. If user opens explorer's search dialog and perform search, a search expression is added into SearchHistory and lastSelected SearchPattern is setted. The event is fired, editor can retrieve lastSelected SearchPattern and in accordance with its parameters it can highlight(in yellow) all matched patterns. If editor dialog is open, it contains shareable SearchHistory. Another direction is search in editor, that adds a SearchPattern in SearchHistory, thus the new item is available also in explorer's search dialog. I would like to ask for review of the proposed solution. Thanks. I am attaching diff with desired API changes + new tests.
Created attachment 19557 [details] diff of desired API changes
Martin you need to reassign the issue and mark it with appropriate keywords if you want a real API review, not just put apireviews on CC. See: http://openide.netbeans.org/tutorial/review-steps.html
Thanks Jesse. I hope Fast-Track should be enough here.
There is an error in previous diff. Missing return statement in SearchHistory.java line 91. I am attaching a corrected diff for completeness.
Created attachment 19594 [details] Corrected diff. Thanks to Ondra for finding this.
Please use diff -u for readability whenever producing patches. Also tip: pass -q to cvs (i.e. "cvs -q di -u ....") to suppress irrelevant messages about folders being traversed. Style/consistency: use List/*<SearchPattern>*/ rather than List/*SearchPattern*/ (will be easier to find when we move to generics). LAST_SELECTED_SEARCH_PATTERN should be "lastSelected" for consistency with JavaBeans conventions, I guess. Otherwise looks fine to me.
I'd like to ask for a little change: could you please define SearchPattern.equals (and hashCode) that compare the values of the objects? Thanks.
yes, it's a good idea. i will implement it. Thanks.
Created attachment 19662 [details] SearchPattern methods equals and hashCode + tests implemented
I am about to commit the proposed changes to CVS.
One last bit from me. I've just looked at the impls of equals and hashCode and it looks like you don't expect SearchPattern.searchExpression to be null. If that's the case, it might be a good idea to assert this in the constructor to make this assumption sound and clear to the users (like me ;-)
Thanks Ondra. implemented: public static SearchPattern create(String searchExpression, boolean wholeWords, boolean matchCase, boolean regExp){ assert searchExpression != null : "searchExpression cannot be null"; // NOI18N return new SearchPattern(searchExpression, wholeWords, matchCase, regExp); }
Never use assertions to check incoming arguments to an API method! You must use proper exceptions (e.g. IllegalArgumentException), which should be listed in the method's throws clause and mentioned in a @throws tag in Javadoc. As an exception, *all* NB APIs are expected to forbid null parameters unless they explicitly permit a null parameter and assign it a meaning (similar for return values), so it is not strictly necessary to document that a parameter must be non-null (though it never hurts). More on assertions vs. exceptions: http://java.sun.com/j2se/1.5.0/docs/guide/language/assert.html#usage
Jesse, thanks for the correction. I removed assertion and put a notice into a javadoc. Implemented in [maintrunk] Checking in api/apichanges.xml; /cvs/openidex/api/apichanges.xml,v <-- apichanges.xml new revision: 1.3; previous revision: 1.2 done Processing log script arguments... More commits to come... RCS file: /cvs/openidex/src/org/openidex/search/SearchHistory.java,v done Checking in src/org/openidex/search/SearchHistory.java; /cvs/openidex/src/org/openidex/search/SearchHistory.java,v <-- SearchHistory.java initial revision: 1.1 done RCS file: /cvs/openidex/src/org/openidex/search/SearchPattern.java,v done Checking in src/org/openidex/search/SearchPattern.java; /cvs/openidex/src/org/openidex/search/SearchPattern.java,v <-- SearchPattern.java initial revision: 1.1 done Processing log script arguments... More commits to come... RCS file: /cvs/openidex/test/unit/src/org/openidex/search/SearchHistoryTest.java,v done Checking in test/unit/src/org/openidex/search/SearchHistoryTest.java; /cvs/openidex/test/unit/src/org/openidex/search/SearchHistoryTest.java,v <-- SearchHistoryTest.java initial revision: 1.1 done
Jesse, you are right and you are not. It's true that if the exception is to be part of the behaviour of the program, than it, of course, has to be checked and thrown "manually" (with if and throw). On the other hand, I doubt that every public method in every api has all it's arguments checked for null and other well-formedness conditions. It may be so both because of typing (the if and throw version is quite verbose to type everywhere) and efficiency reasons. In my oppinion, it's better to have assert, which is not part of the semantics of the api at least for the development phase, then to have nothing and purely undefined behaviour.
Certainly it's better to have an assert than nothing at all, it's just better still to have a typed exception. Possible exception: in some cases it is prohibitively expensive to do a full check on incoming arguments (this is pretty rare but it happens). Then you could use an assert to save time during normal operation (no -ea), or even better use boolean asserts = false; assert asserts = true; if (asserts) { if (!someComplexArgCheck()) { throw new IllegalArgumentException("..."); } }
To be continued on nbdev...