Bug 53110 - openidex.search API change
openidex.search API change
Status: RESOLVED FIXED
Product: utilities
Classification: Unclassified
Component: Search
4.x
PC Windows ME/2000
: P1 (vote)
: 4.x
Assigned To: Martin Roskanin
issues@utilities
: API, API_REVIEW_FAST
Depends on:
Blocks: 51964
  Show dependency treegraph
 
Reported: 2005-01-07 16:05 UTC by Martin Roskanin
Modified: 2005-01-18 11:33 UTC (History)
3 users (show)

See Also:
Issue Type: TASK
:


Attachments
diff of desired API changes (14.32 KB, text/plain)
2005-01-07 16:07 UTC, Martin Roskanin
Details
Corrected diff. Thanks to Ondra for finding this. (14.34 KB, text/plain)
2005-01-11 10:02 UTC, Martin Roskanin
Details
SearchPattern methods equals and hashCode + tests implemented (6.78 KB, text/plain)
2005-01-13 09:41 UTC, Martin Roskanin
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Roskanin 2005-01-07 16:05:11 UTC
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.
Comment 1 Martin Roskanin 2005-01-07 16:07:34 UTC
Created attachment 19557 [details]
diff of desired API changes
Comment 2 Jesse Glick 2005-01-10 21:33:04 UTC
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
Comment 3 Martin Roskanin 2005-01-11 08:58:27 UTC
Thanks Jesse. I hope Fast-Track should be enough here.
Comment 4 Martin Roskanin 2005-01-11 10:00:57 UTC
There is an error in previous diff. Missing return statement in
SearchHistory.java line 91. I am attaching a corrected diff for
completeness.
Comment 5 Martin Roskanin 2005-01-11 10:02:13 UTC
Created attachment 19594 [details]
Corrected diff. Thanks to Ondra for finding this.
Comment 6 Jesse Glick 2005-01-11 20:40:25 UTC
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.
Comment 7 Ondrej Rypacek 2005-01-12 11:26:10 UTC
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.
Comment 8 Martin Roskanin 2005-01-12 16:02:39 UTC
yes, it's a good idea. i will implement it. Thanks.
Comment 9 Martin Roskanin 2005-01-13 09:41:15 UTC
Created attachment 19662 [details]
SearchPattern methods equals and hashCode + tests implemented
Comment 10 Martin Roskanin 2005-01-14 11:00:00 UTC
I am about to commit the proposed changes to CVS.
Comment 11 Ondrej Rypacek 2005-01-14 14:03:00 UTC
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 ;-)


Comment 12 Martin Roskanin 2005-01-14 14:32:40 UTC
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);
    }
Comment 13 Jesse Glick 2005-01-14 17:25:56 UTC
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
Comment 14 Martin Roskanin 2005-01-17 13:53:53 UTC
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
Comment 15 Ondrej Rypacek 2005-01-17 14:04:02 UTC
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. 
Comment 16 Jesse Glick 2005-01-17 17:11:15 UTC
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("...");
    }
}
Comment 17 Ondrej Rypacek 2005-01-18 11:33:27 UTC
To be continued on nbdev...


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