Hi guys, I'm currently working on Ruby bindings for the service method stuff, and I noticed something's strange.
See eg http://repo.or.cz/w/xmms2-tutorial.git?a=blob;f=c/service_client/recursion.c;h=50d4af56a3e836dfb07ffec31707f5ef5f3195c6;hb=HEAD as an example of a service method. atm, service methods get passed an xmmsc_result_t*, that is only used to check for errors (xmmsc_result_iserror, xmmsc_result_get_error) and to get the arguments to the service method (xmmsc_result_get_dict_entry_...). It's also used to return a value from the service method, as it is passed to xmmsc_service_return(). I believe it is *very* wrong to pass an xmmsc_result_t* there! In libxmmsclient, results are used as handles to the xmmsc_blah functions, all of trigger some action in the server and return some data. Results can be handled sync or async, that's why xmmsc_result_notifier_set is defined for *all* of them. This just doesn't apply to the argument to the service method. What we get there isn't a common result, but some strange bastard ;) It makes no sense at all to call xmmsc_result_notifier_set on it, for example, or xmmsc_result_wait, or xmmsc_result_get_value. It also complicates the code in the Ruby bindings for me (and maybe other binding authors? :) Do you agree with me so far? So my point is that we shouldn't pass a common result to the service method callback. The quickest solution that I can think of is implemented in the attached patch. With that patch, we still pass a result, but the client author doesn't know about it ;D Comment, please! The second nitpick I found is how a value is returned from a service method. atm, you do this: result2 = xmmsc_service_return (conn, res, method); xmmsc_result_unref (result2); Can't we just make the service method callbacks return eg SERVICE_METHOD_HANDLED, which results in the above two calls? If there should be no return value, they could return some other nice value. Maybe we could get away with a boolean, too. Seems the current code is more complicated than it could be. Anyway, let me know what you think. Regards, Tilman -- A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? A: Top-posting. Q: What is the most annoying thing on usenet and in e-mail?
diff --git a/src/clients/lib/ruby/rb_xmmsclient.c b/src/clients/lib/ruby/rb_xmmsclient.c index 8d65f7a..b470eb2 100644 --- a/src/clients/lib/ruby/rb_xmmsclient.c +++ b/src/clients/lib/ruby/rb_xmmsclient.c @@ -23,6 +23,7 @@ #include "rb_xmmsclient.h" #include "rb_playlist.h" #include "rb_collection.h" +#include "rb_service_method.h" #include "rb_result.h" #define METHOD_HANDLER_HEADER \ @@ -1555,4 +1556,5 @@ Init_Client (VALUE mXmms) Init_Result (mXmms); cPlaylist = Init_Playlist (mXmms); Init_Collection (mXmms); + Init_ServiceMethod (mXmms); } diff --git a/src/clients/lib/xmmsclient/service.c b/src/clients/lib/xmmsclient/service.c index 9efe092..600df06 100644 --- a/src/clients/lib/xmmsclient/service.c +++ b/src/clients/lib/xmmsclient/service.c @@ -301,16 +301,18 @@ xmmsc_service_request (xmmsc_connection_t *conn, const char *service, * All return values will be reset after this call. * * @param conn The connection to the server. - * @param res The result containing the service cookie. + * @param data The magic cookie thing. * @param method The #xmmsc_service_method_t structure. */ xmmsc_result_t * -xmmsc_service_return (xmmsc_connection_t *conn, xmmsc_result_t *res, +xmmsc_service_return (xmmsc_connection_t *conn, + xmmsc_service_notifier_data_t *data, xmmsc_service_method_t *method) { xmms_ipc_msg_t *msg; uint32_t cookie; x_list_t *n; + xmmsc_result_t *res = (xmmsc_result_t *) data; xmmsc_service_argument_t *arg; x_check_conn (conn, NULL); @@ -1288,7 +1290,49 @@ dispatch (xmmsc_result_t *res, void *data) x_return_if_fail (method); - method->func (method->conn, res, method, method->udata); + method->func (method->conn, (xmmsc_service_notifier_data_t *) res, + method, method->udata); +} + +int +xmmsc_service_notifier_data_iserror (xmmsc_service_notifier_data_t *data) +{ + return xmmsc_result_iserror ((xmmsc_result_t *) data); +} + +const char * +xmmsc_service_notifier_data_get_error (xmmsc_service_notifier_data *data) +{ + return xmmsc_result_get_error ((xmmsc_result_t *) data); +} + +int +xmmsc_service_notifier_data_get_arg_string ( + xmmsc_service_notifier_data_t *data, const char *key, char **r) +{ + return xmmsc_result_get_dict_entry_string ((xmmsc_result_t *) data, key, r); +} + +int +xmmsc_service_notifier_data_get_arg_int ( + xmmsc_service_notifier_data_t *data, const char *key, int32_t *r) +{ + return xmmsc_result_get_dict_entry_int ((xmmsc_result_t *) data, key, r); +} + +int +xmmsc_service_notifier_data_get_arg_uint ( + xmmsc_service_notifier_data_t *data, const char *key, uint32_t *r) +{ + return xmmsc_result_get_dict_entry_uint ((xmmsc_result_t *) data, key, r); +} + +int +xmmsc_service_notifier_data_get_arg_collection ( + xmmsc_service_notifier_data_t *data, const char *key, + xmmsc_coll_t **coll) +{ + return xmmsc_result_get_dict_entry_collection ((xmmsc_result_t *) data, key, coll); } /* @} */ diff --git a/src/include/xmmsclient/xmmsclient.h b/src/include/xmmsclient/xmmsclient.h index fd7f303..7dda254 100644 --- a/src/include/xmmsclient/xmmsclient.h +++ b/src/include/xmmsclient/xmmsclient.h @@ -288,7 +288,8 @@ typedef void (*xmmsc_result_notifier_t) (xmmsc_result_t *res, void *user_data); /* * Service Client */ -typedef void (*xmmsc_service_notifier_t) (xmmsc_connection_t *conn, xmmsc_result_t *res, xmmsc_service_method_t *method, void *user_data); +typedef void xmmsc_service_notifier_data_t; +typedef void (*xmmsc_service_notifier_t) (xmmsc_connection_t *conn, xmmsc_service_notifier_data_t *data, xmmsc_service_method_t *method, void *user_data); xmmsc_result_t *xmmsc_service_register (xmmsc_connection_t *conn, const char *name, const char *description, uint32_t major, uint32_t minor); xmmsc_result_t *xmmsc_service_method_register (xmmsc_connection_t *conn, const char *service, xmmsc_service_method_t *method); @@ -309,6 +310,11 @@ xmmsc_result_t *xmmsc_broadcast_service_shutdown (xmmsc_connection_t *c); xmmsc_service_method_t *xmmsc_service_method_new (const char *name, const char *description, xmmsc_service_notifier_t func, void *user_data); xmmsc_service_method_t *xmmsc_service_method_new_full (const char *name, const char *description, xmmsc_service_notifier_t func, void *user_data, xmmsc_user_data_free_func_t free_func); +int xmmsc_service_notifier_data_get_arg_string (xmmsc_service_notifier_data_t *data, const char *key, char **r); +int xmmsc_service_notifier_data_get_arg_int (xmmsc_service_notifier_data_t *data, const char *key, int32_t *r); +int xmmsc_service_notifier_data_get_arg_uint (xmmsc_service_notifier_data_t *data, const char *key, uint32_t *r); +int xmmsc_service_notifier_data_get_arg_collection (xmmsc_service_notifier_data_t *data, const char *key, xmmsc_coll_t **coll); + /* * RESULTS */
pgpAf9qwA7j9Z.pgp
Description: PGP signature
-- _______________________________________________ Xmms2-devel mailing list Xmms2-devel@lists.xmms.se http://lists.xmms.se/cgi-bin/mailman/listinfo/xmms2-devel