Community
Participate
Working Groups
It would be nice if we could have ECF support XMPP search so we can help people find users if their server supports it. http://www.igniterealtime.org/projects/openfire/plugins/search/readme.html http://xmpp.org/extensions/xep-0055.html
Just for everyone's info: even the old version of Smack that we are currently using (2.2.1) has some classes that implement xep-0055. I'm sure the newer versions of Smack (i.e. 3.X) also support it. One question would be...should we attempt to add on an API to the presence API to allow for search (at the API level)? It could be implemented as an adapter off of IRosterManager (e.g.).
I guess it makes sense we have it at the API level (defining a simple API for search), and then other providers could implement this feature also
Created attachment 120735 [details] Search Interfaces API A patch that contains the Interfaces for a possible simple search API
Keeping in mind the idea to have it at the API level, I defined a simple API for search where each provider can implement your own. I put a patch that contains these Interfaces and some implementations for your comments. Please let me know your suggestions. In this meantime I'm implementing a XMPP user search to 'attest' this search API.
Trying to validate the user search API, I implemented the XMPP user search. This implementation is distributed on the branches below: * presence_user_search * tests_presence_user_search * xmpp_user_search * xmpp_ui_user_search * tests_xmpp_user_search For the tests I used the ecf.eclipse.org XMPP server and the tests ran in a successfully way. Please, take a look into this user search API and put your comments on this bug. Now I'm starting the XMPP user search UI implementation. See bug 259531
Created attachment 122604 [details] User search API User Search API including the XMPP implementation for user search.
The user search on XMPP provider is now available, this search can happen in two different ways, sync or async. It is possible to add the contact after you find it in the search. The code developed for the User search API is on the branch presence_user_search. I intend to merge this code into HEAD this week...for presence API, xmpp provider impl, ui, so I appreciate any comments on that. Trying to make it as easy as possible for to look at/comment about quickly/easily, I created a patch and attach it to bug.
Created attachment 123016 [details] User Seach API Adding more tests and make an improvement on the listener and event.
(In reply to comment #8) > Created an attachment (id=123016) [details] > User Seach API > > Adding more tests and make an improvement on the listener and event. > Hi Marcelo. This contribution looks great. Thanks so much Marcelo for all the work...we're getting there very rapidly, I think. Some comments and questions: In IUserSearchManager API: You've got two search methods...i.e. ISearch search(ICriteria criteria) throws ContainerConnectException; and void search(IUserSearchListener) First, questions about ISearch search(ICriteria criteria): 1) why throw ContainerConnectException? Rather than use this exception, I would suggest creating a new exception class (e.g. UserSearchException) that extends ECFException...and possibly adds access to the criteria associated with the search (e.g. UserSearchException.getSearchCriteria()) 2) Doesn't the async search need to be given an ICriteria like the synchronous search(ICriteria). That is, how is the client going to specify the criteria for the asynchronous search if not as a parameter to the search method? e.g. public void search(ICriteria criteria, IUserSearchListener listener); 3) I would suggest that the IUserSearchListener have just one method instead of two: public void handleUserSearchEvent(IUserSearchEvent event); with IUserSearchEvent declaring a single method: ICriteria getCriteria(); Also I suggest that there be different sub-interfaces of IUserSearchEvent to represent different types of events...e.g. class IUserSearchCompleteEvent ISearch getSearch(); It may be, for example, that there is *only* IUserSearchCompleteEvent to begin with...and other interfaces added later. Things look good Marcelo...let's get these things worked out and we'll get the API and xmpp impl into ECF 3.0M5. Thanks.
(In reply to comment #8) > Created an attachment (id=123016) [details] I'm eyeballing this since I haven't applied the patch. I'm not convinced that there's any value in returning a non-null IUserSearchManager implementation for the MSN provider if it's not going to do anything. I don't particularly like the name ISelection as it will conflict with JFace's ISelection interface. Can something else be introduced instead? XMPPCriteria's toString() method doesn't appear to close its square brackets.
(In reply to comment #10) > (In reply to comment #8) > > Created an attachment (id=123016) [details] [details] > > I'm eyeballing this since I haven't applied the patch. > > I'm not convinced that there's any value in returning a non-null > IUserSearchManager implementation for the MSN provider if it's not going to do > anything. The idea is to use a defined neutral behavior, and furthermore implementing the IUserSearchManager#isEnable() is an "interesting" way to advice the menu doesn't consider the "User Search" feature. > > I don't particularly like the name ISelection as it will conflict with JFace's > ISelection interface. Can something else be introduced instead? You are right... what do you think about IRestriction? > > XMPPCriteria's toString() method doesn't appear to close its square brackets. I fixed it. >
(In reply to comment #9) > (In reply to comment #8) > > Created an attachment (id=123016) [details] [details] > > User Seach API > > > > Adding more tests and make an improvement on the listener and event. > > > > Hi Marcelo. This contribution looks great. Thanks so much Marcelo for all the > work...we're getting there very rapidly, I think. Thanks Scott. > > Some comments and questions: > > In IUserSearchManager API: > > You've got two search methods...i.e. > > ISearch search(ICriteria criteria) throws ContainerConnectException; > > and > > void search(IUserSearchListener) > > First, questions about ISearch search(ICriteria criteria): > > 1) why throw ContainerConnectException? Rather than use this exception, I > would suggest creating a new exception class (e.g. UserSearchException) that > extends ECFException...and possibly adds access to the criteria associated with > the search (e.g. UserSearchException.getSearchCriteria()) I'm adding it. > > 2) Doesn't the async search need to be given an ICriteria like the synchronous > search(ICriteria). That is, how is the client going to specify the criteria > for the asynchronous search if not as a parameter to the search method? e.g. > > public void search(ICriteria criteria, IUserSearchListener listener); In a first moment I implemented this way, and after that I moved the ICriteria for event (IUserSearchEvent), but you are right, make sense has it seems simpler to make the asynchronous/non-blocking call look/be as much like the blocking call: search(ICriteria) as possible...that is: search(ICriteria,IUserSearchListener). > > 3) I would suggest that the IUserSearchListener have just one method instead of > two: > > public void handleUserSearchEvent(IUserSearchEvent event); > Ok. > with IUserSearchEvent declaring a single method: > > ICriteria getCriteria(); Then this event (IUserSearchEvent) is just to call the IUserSearchListener#handleUserSearchEvent(event) for example... > > Also I suggest that there be different sub-interfaces of IUserSearchEvent to > represent different types of events...e.g. > > class IUserSearchCompleteEvent > > ISearch getSearch(); Perfect, I will create this event to notify the clients about the search when it is complete, keeping the ISearch into that this event. > > It may be, for example, that there is *only* IUserSearchCompleteEvent to begin > with...and other interfaces added later. > > Things look good Marcelo...let's get these things worked out and we'll get the > API and xmpp impl into ECF 3.0M5. > > Thanks. >
Created attachment 123308 [details] User Seach API applying changes commented on #9 and #10
Hi Marcelo. This is looking good. A few very minor comments. I see that you renamed ISelection to IRestriction as per comment #10. But the IUserSearchManager.createSelection() should probably then be renamed to createRestriction(). WRT Javadocs...would be good if you could add some docs in IUserSearchManager.search/1 and search/2 for the 'criteria' parameter (e.g. is it ok to be null in particular). Typo in javadocs for search/2: "This method can apply search to math the specific criteria..." 'math' should be 'match' I think. For this method: public String[] getUserPropertiesFields() throws ContainerConnectException; Why would it be ContainerConnectException? Should it be ECFException instead (e.g. to allow for exceptions in the blocking call rather than the connect?). If getUserPropertiesFields() can block, would be good to note this in javadocs. In AbstractUserSearchListener the UserListener class code (i.e. the imple of handleUserSearchEvent(IUserSearchEvent) doesn't seem quite right as: 1) You currently have the code in the if (event instanceof IUserSearchCompleteEvent) commented out 2) The conditional 'else if (event instanceof IUserSearchEvent)' is not needed (given that the IUserSearchEvent is the type passed into handleUserSearchEvent). 3) the catch (UserSearchException e) { e.printStackTrace() } isn't a very good idea :). 4) Would it make sense to make fireUserSearchEvent() protected (rather than public), as I think it's intended for subclasses, correct? 5) I don't see the use for the 'criteria' member on AbstractUserSearchManager...or how it's set to begin with. Might want to remove it. With these fixes/changes and ones that others identify, I think things are looking close to ready for releasing/merging to HEAD for M5. When you are ready to do this please coordinate with me, as there are other projects that implement the presence API that have to be updated so as to compile in our daily build (so checking in these additions without updating the other projects will break the osu daily build...i.e. yahoo, twitter, skype, aim providers).
(In reply to comment #14) > Hi Marcelo. This is looking good. > > A few very minor comments. > > I see that you renamed ISelection to IRestriction as per comment #10. But the > IUserSearchManager.createSelection() should probably then be renamed to > createRestriction(). Fixed. > > WRT Javadocs...would be good if you could add some docs in > IUserSearchManager.search/1 and search/2 for the 'criteria' parameter (e.g. is > it ok to be null in particular). > > Typo in javadocs for search/2: > > "This method can apply search to math the specific criteria..." > > 'math' should be 'match' I think. > Thanks, I improved the comments there and "fixed" the typo. > For this method: > > public String[] getUserPropertiesFields() throws ContainerConnectException; > > Why would it be ContainerConnectException? Should it be ECFException instead > (e.g. to allow for exceptions in the blocking call rather than the connect?). > yes. > If getUserPropertiesFields() can block, would be good to note this in javadocs. fixed. > > In AbstractUserSearchListener the UserListener class code (i.e. the imple of > handleUserSearchEvent(IUserSearchEvent) doesn't seem quite right as: > > 1) You currently have the code in the if (event instanceof > IUserSearchCompleteEvent) commented out > > 2) The conditional 'else if (event instanceof IUserSearchEvent)' is not needed > (given that the IUserSearchEvent is the type passed into handleUserSearchEvent). ops you are right :) > > 3) the catch (UserSearchException e) { e.printStackTrace() } isn't a very good > idea :). ops "again"... > > 4) Would it make sense to make fireUserSearchEvent() protected (rather than > public), as I think it's intended for subclasses, correct? sure > > 5) I don't see the use for the 'criteria' member on > AbstractUserSearchManager...or how it's set to begin with. Might want to remove > it. it was removed and the search method deal with it > > With these fixes/changes and ones that others identify, I think things are > looking close to ready for releasing/merging to HEAD for M5. When you are ready > to do this please coordinate with me, as there are other projects that implement > the presence API that have to be updated so as to compile in our daily build (so > checking in these additions without updating the other projects will break the > osu daily build...i.e. yahoo, twitter, skype, aim providers). I updated the ecf1 projects (providers) to support the user search api. The code was changed following your suggest/comments. Everything is commited on the branch.
I released the changes to HEAD and that new bugs should be created for user search API and/or implementation bugs.
