|
Description
Debarshi Ray
2011-02-27 08:55:18 UTC
Created attachment 43889 [details] [review] Initial patch Created attachment 43906 [details] [review] Second iteration of earlier patch + Fixes a build failure due to a last minute change in the earlier patch. + Instead of directly returning from a IDLE_PARSER_NUMERIC_WHOISUSER message, it waits for a IDLE_PARSER_NUMERIC_ENDOFWHOIS message. That way it would be possible to return more information than just what is available in a IDLE_PARSER_NUMERIC_WHOISUSER message. Review of attachment 43906 [details] [review]: Great! One functional issue, and a few nitpicks: ::: src/idle-connection.c @@ +77,3 @@ G_DEFINE_TYPE_WITH_CODE(IdleConnection, idle_connection, TP_TYPE_BASE_CONNECTION, G_IMPLEMENT_INTERFACE(TP_TYPE_SVC_CONNECTION_INTERFACE_ALIASING, _aliasing_iface_init); + G_IMPLEMENT_INTERFACE(TP_TYPE_SVC_CONNECTION_INTERFACE_CONTACT_INFO, idle_contact_info_iface_init); I think you also want to add TP_IFACE_CONNECTION_INTERFACE_CONTACT_INFO to interfaces_always_present later in the file. This will make it appear in the Connection's Interfaces property <http://telepathy.freedesktop.org/spec/Connection.html#Property:Interfaces> which the client-side bindings use to determine whether or not an interface is implemented. Without this, clients may not realise that they can use these methods. ::: src/idle-contact-info.c @@ +48,3 @@ + */ +static void _insert_contact_field(GPtrArray *contact_info, const gchar *field_name, const gchar * const *field_params, const gchar * const *field_values) { + gchar *field_name_down = g_ascii_strdown (field_name, -1); Nitpick: given that we only ever pass strings provided by Idle itself as 'field_name', can't we just assume they're lower-case (or else the code calling _insert_contact_field is buggy)? @@ +108,3 @@ + + tp_svc_connection_interface_contact_info_return_from_refresh_contact_info(context); +} You could just omit the stub implementations of GetContactInfo, RefreshContactInfo and SetContactInfo entirely: tp-glib would fall back to returning a NotImplemented error. @@ +178,3 @@ + g_error_free(error); + return; + } If tp_handle_is_valid said that 'contact' was valid, tp_handle_inspect() on it will always return non-NULL. ::: src/idle-contact-info.h @@ +35,3 @@ +extern TpDBusPropertiesMixinPropImpl *idle_contact_info_properties; +void idle_contact_info_properties_getter (GObject *object, GQuark interface, + GQuark name, GValue *value, gpointer getter_data); Looks like you didn't actually implement the two properties: <http://telepathy.freedesktop.org/spec/Connection_Interface_Contact_Info.html#properties>. But since their values would be '0' and '[]' anyway, this doesn't seem like a big deal. :) (In reply to comment #3) > Review of attachment 43906 [details] [review]: > > Great! One functional issue, and a few nitpicks: > > ::: src/idle-connection.c > @@ +77,3 @@ > G_DEFINE_TYPE_WITH_CODE(IdleConnection, idle_connection, > TP_TYPE_BASE_CONNECTION, > G_IMPLEMENT_INTERFACE(TP_TYPE_SVC_CONNECTION_INTERFACE_ALIASING, > _aliasing_iface_init); > + G_IMPLEMENT_INTERFACE(TP_TYPE_SVC_CONNECTION_INTERFACE_CONTACT_INFO, > idle_contact_info_iface_init); > > I think you also want to add TP_IFACE_CONNECTION_INTERFACE_CONTACT_INFO to > interfaces_always_present later in the file. Done. > ::: src/idle-contact-info.c > @@ +48,3 @@ > + */ > +static void _insert_contact_field(GPtrArray *contact_info, const gchar > *field_name, const gchar * const *field_params, const gchar * const > *field_values) { > + gchar *field_name_down = g_ascii_strdown (field_name, -1); > > Nitpick: given that we only ever pass strings provided by Idle itself as > 'field_name', can't we just assume they're lower-case (or else the code calling > _insert_contact_field is buggy)? You are right. I had thought about it too. Thing is that I copied the function and the comments above it verbatim from telepathy-gabble. Apart from some style fixes I did not change the code to avoid fragmented copies of the same code. But I have no strong opinion about this. :-) > @@ +108,3 @@ > + > + > tp_svc_connection_interface_contact_info_return_from_refresh_contact_info(context); > +} > > You could just omit the stub implementations of GetContactInfo, > RefreshContactInfo and SetContactInfo entirely: tp-glib would fall back to > returning a NotImplemented error. I intend to implement those methods. :-) > @@ +178,3 @@ > + g_error_free(error); > + return; > + } > > If tp_handle_is_valid said that 'contact' was valid, tp_handle_inspect() on it > will always return non-NULL. Done. > ::: src/idle-contact-info.h > @@ +35,3 @@ > +extern TpDBusPropertiesMixinPropImpl *idle_contact_info_properties; > +void idle_contact_info_properties_getter (GObject *object, GQuark interface, > + GQuark name, GValue *value, gpointer getter_data); > > Looks like you didn't actually implement the two properties: Ok. I will look into this. Created attachment 43927 [details] [review] Third iteration of earlier patch + It addresses some of the issues raised in the earlier review. + RequestContactInfo returns the away message and idle time in addition to the real name. x-irc-away and x-irc-idle are used as the vcard field names. Created attachment 44043 [details] [review] Fourth iteration of earlier patch + Implemented the properties: ContactInfoFlags, SupportedFields + x-presence-type, x-presence-status-identifier, x-presence-status-message are used in a manner analogous to http://telepathy.freedesktop.org/spec/Connection_Interface_Simple_Presence.html + Worked around a GIMPNet "bug" where the away is status of a nick that is not connected to the same server as you is only returned when "WHOIS foo foo" is issued. "WHOIS foo" does not return the away status if both are not on the same server. Created attachment 44074 [details] [review] Fifth iteration of the earlier patch This is a minor improvement to basically ensure that _return_from_request_contact_info is called only if there is a valid request->contact_info. The NULL pointer check is now done in _end_whois_handler. Created attachment 44075 [details] [review] Handle ERR_NOSUCHSERVER Created attachment 44566 [details] [review] Handle RPL_WHOISCHANNELS messages in response to RequestContactInfo RFC 2812 (http://tools.ietf.org/html/rfc2812#section-5.1) defines RPL_WHOISCHANNELS as: "<nick> :*( ( "@" / "+" ) <channel> " " )" Preliminary testing (IRCNet and GIMPNet) suggests that it is actually: "<nick> :1*( ( "@" / "+" ) <channel> " " )" To maintain strict compliance with the RFC I defined the format as IIIc., but I am not sure if anything strange might happen if we get a "blank" RPL_WHOISCHANNELS from some server. Also, how will g_strsplit behave with Unicode? Channels can have non-ASCII even though the RFC (http://tools.ietf.org/html/rfc2812#section-2.3.1) does not seem to explicitly say so. Created attachment 44584 [details] [review] Handle RPL_WHOISLOGGEDIN messages in response to RequestContactInfo Created attachment 44601 [details] [review] Fix the RPL_WHOISCHANNELS patch Instead of: ["x-irc-channels"], [], ["#telepathy", "#freenode"] do: ["x-irc-channel"], [], ["#telepathy"] ["x-irc-channel"], [], ["#freenode"] Review of attachment 44074 [details] [review]: ::: src/idle-contact-info.c @@ +49,3 @@ + */ +static void _insert_contact_field(GPtrArray *contact_info, const gchar *field_name, const gchar * const *field_params, const gchar * const *field_values) { + gchar *field_name_down = g_ascii_strdown (field_name, -1); I'd still just use field_name directly, and make sure nothing passes uppercase strings to this function (as nothing does, so that's fine). @@ +136,3 @@ + ContactInfoRequest *request; + + request = g_new0(ContactInfoRequest, 1); As a general rule, when allocating fixed-sized structs use g_slice_new0() and g_slice_free(). (Obviously this code is not performance-critical! ☺) @@ +181,3 @@ + TP_BASE_CONNECTION_ERROR_IF_NOT_CONNECTED(TP_BASE_CONNECTION(iface), context); + tp_svc_connection_interface_contact_info_return_from_set_contact_info(context); +} Just leave SetContactInfo entirely unimplemented. Then telepathy-glib will return a NotImplemented error on your behalf. @@ +292,3 @@ + g_value_set_static_boxed(value, getter_data); + else + g_value_set_uint(value, GPOINTER_TO_UINT(getter_data)); Does NULL really work as SupportedFields? It's meant to be a GPtrArray of GValues, and part of me would expect dbus-glib to crash if you use NULL instead. (It took me a few moments to figure this out; maybe it would be clearer to define the values in this function, rather than stashing them in getter_data and casting it.) Review of attachment 44075 [details] [review]: Looks mostly good. ::: src/idle-contact-info.c @@ +269,3 @@ + g_value_array_prepend(norm_args, &value); + + * caused by us, and we do not want to handle it. These contortions make it look like _is_valid_response() should be changed to take a TpHandle as the second argument, rather than having to fake up a GValueArray which it just grabs the first element of. @@ +274,3 @@ + request = g_queue_peek_head(conn->contact_info_requests); + + * To check this we map the value of the <server name> to a handle and see if it matches the handle for which we had made the request. The fact that the error is “no such server” is an IRC protocol oddity, and I imagine this will confuse people reading dbus-monitor! Maybe the message could be something like “User '%s' unknown; they may have disconnected”? Review of attachment 44584 [details] [review]: Looks good. Review of attachment 44601 [details] [review]: Looks good. (In reply to comment #12) > Review of attachment 44074 [details] [review]: > > ::: src/idle-contact-info.c > @@ +49,3 @@ > + */ > +static void _insert_contact_field(GPtrArray *contact_info, const gchar > *field_name, const gchar * const *field_params, const gchar * const > *field_values) { > + gchar *field_name_down = g_ascii_strdown (field_name, -1); > > I'd still just use field_name directly, and make sure nothing passes uppercase > strings to this function (as nothing does, so that's fine). Done. > @@ +136,3 @@ > + ContactInfoRequest *request; > + > + request = g_new0(ContactInfoRequest, 1); > > As a general rule, when allocating fixed-sized structs use g_slice_new0() and > g_slice_free(). (Obviously this code is not performance-critical! ☺) Done. > @@ +181,3 @@ > + TP_BASE_CONNECTION_ERROR_IF_NOT_CONNECTED(TP_BASE_CONNECTION(iface), > context); > + > tp_svc_connection_interface_contact_info_return_from_set_contact_info(context); > +} > > Just leave SetContactInfo entirely unimplemented. Then telepathy-glib will > return a NotImplemented error on your behalf. Done. Also removed the GetContactInfo, RefreshContactInfo stubs. > @@ +292,3 @@ > + g_value_set_static_boxed(value, getter_data); > + else > + g_value_set_uint(value, GPOINTER_TO_UINT(getter_data)); > > Does NULL really work as SupportedFields? It's meant to be a GPtrArray of > GValues, and part of me would expect dbus-glib to crash if you use NULL > instead. > > (It took me a few moments to figure this out; maybe it would be clearer to > define the values in this function, rather than stashing them in getter_data > and casting it.) I am a bit clueless about properties and was just imitating Gabble code. I will try to look at it more closely. Created attachment 44695 [details] [review] Implement ContactInfo (In reply to comment #13) > Review of attachment 44075 [details] [review]: > ::: src/idle-contact-info.c > @@ +269,3 @@ > + g_value_array_prepend(norm_args, &value); > + > + * caused by us, and we do not want to handle it. > > These contortions make it look like _is_valid_response() should be changed to > take a TpHandle as the second argument, rather than having to fake up a > GValueArray which it just grabs the first element of. That is true. I never thought about it. On the other hand, this ERR_NOSUCHSERVER is an exception in the sense that all the other numeric replies to a WHOIS will have a handle as the first element. So by passing the whole GValueArray we can avoid having the following statement repeated in all the handlers: TpHandle handle = g_value_get_uint(g_value_array_get_nth(args, 0)); Having said that, even now the following snippet is repeated in almost every handler: request = g_queue_peek_head(conn->contact_info_requests); if (request->contact_info == NULL) request->contact_info = dbus_g_type_specialized_construct(TP_ARRAY_TYPE_CONTACT_INFO_FIELD_LIST); I have no preferences. :-) > @@ +274,3 @@ > + request = g_queue_peek_head(conn->contact_info_requests); > + > + * To check this we map the value of the <server name> to a handle and see > if it matches the handle for which we had made the request. > > The fact that the error is “no such server” is an IRC protocol oddity, and I > imagine this will confuse people reading dbus-monitor! Maybe the message could > be something like “User '%s' unknown; they may have disconnected”? Done. Created attachment 44696 [details] [review] Handle ERR_NOSUCHSERVER (In reply to comment #16) > (In reply to comment #12) > > Review of attachment 44074 [details] [review] [details]: > > @@ +292,3 @@ > > + g_value_set_static_boxed(value, getter_data); > > + else > > + g_value_set_uint(value, GPOINTER_TO_UINT(getter_data)); > > > > Does NULL really work as SupportedFields? It's meant to be a GPtrArray of > > GValues, and part of me would expect dbus-glib to crash if you use NULL > > instead. > > > > (It took me a few moments to figure this out; maybe it would be clearer to > > define the values in this function, rather than stashing them in getter_data > > and casting it.) > I am a bit clueless about properties and was just imitating Gabble code. I will > try to look at it more closely. I have fixed it now to return an empty GPtrArray. I have also taken this opportunity to not use the 'getter_data' field, and just directly return the value of the property. Makes the code a bit easier to read in my opinion. Created attachment 45013 [details] [review] Implement ContactInfo (In reply to comment #18) > On the other hand, this ERR_NOSUCHSERVER is an exception in the sense that all > the other numeric replies to a WHOIS will have a handle as the first element. > So by passing the whole GValueArray we can avoid having the following statement > repeated in all the handlers: > TpHandle handle = g_value_get_uint(g_value_array_get_nth(args, 0)); You're right; I think faking up the GValueArray is arguably neater. :) I've merged these four patches: <http://cgit.freedesktop.org/telepathy/telepathy-idle/log/?id=5c3672d0d66eb9f51ef1c86d287d637b5d9f69d9>. As discussed on IRC, it would be good to handle RPL_TRYAGAIN by returning an error (such as ServiceBusy) from RequestContactInfo. Created attachment 45989 [details] [review] Handle RPL_WHOISSERVER messages in response to RequestContactInfo I don't know if x-irc-server[-info] is the best name for the vCard field or not. (In reply to comment #23) > Created an attachment (id=45989) [details] > Handle RPL_WHOISSERVER messages in response to RequestContactInfo > > I don't know if x-irc-server[-info] is the best name for the vCard field or > not. Or maybe we should do: ['x-irc-server'], [], ['lindbohm.freenode.net', 'Stockholm, Sweden'], or ['x-irc-whoisserver'], [], ['lindbohm.freenode.net', 'Stockholm, Sweden'], instead of ['x-irc-server'], [], ['lindbohm.freenode.net', and ['x-irc-server-info'], [], ['Stockholm, Sweden'] What do you say? (In reply to comment #24) > (In reply to comment #23) > > Created an attachment (id=45989) [details] [details] > > Handle RPL_WHOISSERVER messages in response to RequestContactInfo > > > > I don't know if x-irc-server[-info] is the best name for the vCard field or > > not. > > Or maybe we should do: > ['x-irc-server'], [], ['lindbohm.freenode.net', 'Stockholm, Sweden'], or > What do you say? This would get my vote. Created attachment 46064 [details] [review] Handle RPL_TRYAGAIN messages in response to RequestContactInfo Created attachment 46065 [details] [review] Handle WHOIS messages in our test IRC server Created attachment 46066 [details] [review] constants.py: add SERVICE_BUSY Created attachment 46067 [details] [review] Test to cover RequestContactInfo (In reply to comment #25) > (In reply to comment #24) > > (In reply to comment #23) > > > Created an attachment (id=45989) [details] [details] [details] > > > Handle RPL_WHOISSERVER messages in response to RequestContactInfo > > > > > > I don't know if x-irc-server[-info] is the best name for the vCard field or > > > not. > > > > Or maybe we should do: > > ['x-irc-server'], [], ['lindbohm.freenode.net', 'Stockholm, Sweden'], or > > > What do you say? > > This would get my vote. Sounds good. Review of attachment 46064 [details] [review]: Looks functionally fine, though I have a couple of nitpicks :) ::: src/idle-contact-info.c @@ +256,3 @@ + command = g_ascii_strup(g_value_get_string(g_value_array_get_nth(args, 0)), -1); + if (g_strcmp0(command, "WHOIS")) + * WHOIS. So we assume that the last WHOIS request that we had issued has resulted in this RPL_TRYAGAIN. This is fine as long nobody else is You can use g_ascii_strcasecmp() to avoid having to uppercase the string, which avoids needing the goto cleanup. @@ +262,3 @@ + msg = g_value_get_string(g_value_array_get_nth(args, 1)); + + g_error_new_literal(). Review of attachment 46065 [details] [review]: Looks good — while I am not much enamoured of the way the Idle test suite auto-responds to requests (unlike the Gabble one, say, where the test case generally manually sends the replies), this change fits in well with the current model. Review of attachment 46066 [details] [review]: Looks good. Review of attachment 46067 [details] [review]: Just some tiny nitpicks: ::: tests/twisted/messages/contactinfo-request.py @@ +15,3 @@ + for (name, parameters, value) in vcard: + if name == 'fn': + assert value[0] == 'Test User' Does servicetest.py in Idle have the assertEquals() etc. family of functions that are included in Gabble's version of servicetest.py? If not, okay, but maybe in the future Idle's copy could be refreshed. @@ +39,3 @@ + call_async(q, conn, 'GetSelfHandle') + event = q.expect('dbus-return', method='GetSelfHandle') + self_handle = event.value[0] You can spell this as self_handle = conn.GetSelfHandle() @@ +51,3 @@ + call_async(q, contact_info, 'RequestContactInfo', self_handle) + event = q.expect('dbus-error', method='RequestContactInfo') + assert event.name == SERVICE_BUSY Maybe you could comment this to explain that the test server alternates success and failure? Created attachment 46158 [details] [review] Handle RPL_WHOISSERVER messages in response to RequestContactInfo (In reply to comment #31) > Review of attachment 46064 [details] [review]: > > Looks functionally fine, though I have a couple of nitpicks :) > > ::: src/idle-contact-info.c > @@ +256,3 @@ > + command = g_ascii_strup(g_value_get_string(g_value_array_get_nth(args, > 0)), -1); > + if (g_strcmp0(command, "WHOIS")) > + * WHOIS. So we assume that the last WHOIS request that we had issued has > resulted in this RPL_TRYAGAIN. This is fine as long nobody else is > > You can use g_ascii_strcasecmp() to avoid having to uppercase the string, which > avoids needing the goto cleanup. Fixed. > @@ +262,3 @@ > + msg = g_value_get_string(g_value_array_get_nth(args, 1)); > + > + > > g_error_new_literal(). Fixed. Created attachment 46160 [details] [review] Handle RPL_TRYAGAIN messages in response to RequestContactInfo (In reply to comment #34) > Review of attachment 46067 [details] [review]: > > Just some tiny nitpicks: > > ::: tests/twisted/messages/contactinfo-request.py > @@ +15,3 @@ > + for (name, parameters, value) in vcard: > + if name == 'fn': > + assert value[0] == 'Test User' > > Does servicetest.py in Idle have the assertEquals() etc. family of functions > that are included in Gabble's version of servicetest.py? It does. Fixed to use assertEquals. > @@ +39,3 @@ > + call_async(q, conn, 'GetSelfHandle') > + event = q.expect('dbus-return', method='GetSelfHandle') > + self_handle = event.value[0] > > You can spell this as self_handle = conn.GetSelfHandle() Done. What is the difference? I thought this would cause a synchronous DBus call. > @@ +51,3 @@ > + call_async(q, contact_info, 'RequestContactInfo', self_handle) > + event = q.expect('dbus-error', method='RequestContactInfo') > + assert event.name == SERVICE_BUSY > > Maybe you could comment this to explain that the test server alternates success > and failure? Done. Created attachment 46161 [details] [review] Test to cover RequestContactInfo (In reply to comment #38) > Done. What is the difference? I thought this would cause a synchronous DBus > call. That's exactly what it does… it's just more readable in this context, I think. (Particularly since there's a synchronous call to RequestHandles() below it!) The revised patches look good; commit away! Created attachment 46248 [details] [review] Handle WHOIS messages in our test IRC server Removed a spurious space due to a typo in the 378 numeric. This does not affect anything at the moment because we don't handle it and hence don't test for it at the moment. (In reply to comment #40) > The revised patches look good; commit away! Committed and pushed. Created attachment 46377 [details] [review] Handle RPL_WHOISHOST messages in response to RequestContactInfo Created attachment 46378 [details] [review] idletest: return an empty RPL_WHOISCHANNELS in response to WHOIS Created attachment 46379 [details] [review] Handle empty RPL_WHOISCHANNELS in response to RequestContactInfo Review of attachment 46377 [details] [review]: ::: src/idle-contact-info.c @@ +321,3 @@ + + /* msg == "is connecting from *@<hostname> <IP>" */ + request = g_queue_peek_head(conn->contact_info_requests); You need to check that there are actually more than three elements in the string. Relying on the server not sending malformed data, and crashing if it does, doesn't seem wise. I think I would check whether the string actually has the expected prefix; if it does, strsplit the remainder of the string and put that in the contact info field; if it does not, either just include the whole string verbatim or ignore it? Review of attachment 46378 [details] [review]: Looks fine. Review of attachment 46379 [details] [review]: Looks fine, I guess it fixes a crash exposed by the change to the test case? (In reply to comment #46) > Review of attachment 46377 [details] [review]: > > ::: src/idle-contact-info.c > @@ +321,3 @@ > + > + /* msg == "is connecting from *@<hostname> <IP>" */ > + request = g_queue_peek_head(conn->contact_info_requests); > > You need to check that there are actually more than three elements in the > string. Relying on the server not sending malformed data, and crashing if it > does, doesn't seem wise. > > I think I would check whether the string actually has the expected prefix; if > it does, strsplit the remainder of the string and put that in the contact info > field; if it does not, either just include the whole string verbatim or ignore > it? I chose to check the prefix. Checking it cleverly will ensure that there are atleast 3 elements in the string. Also, as explained in the patch since there is some conflicting (arcane?) use of 378, I chose to bail out if the prefix does not match. It is not our problem if daemons make crazy little changes to the protocol here and there. Created attachment 46403 [details] [review] Handle RPL_WHOISHOST messages in response to RequestContactInfo (In reply to comment #48) > Review of attachment 46379 [details] [review]: > > Looks fine, I guess it fixes a crash exposed by the change to the test case? Yes. Created attachment 46405 [details] [review] Try not to mistake RPL_WHOWAS_TIME as RPL_WHOISLOGGEDIN I feel like herding all ircd developers into a single room and beating each of them with a cluebat. Review of attachment 46403 [details] [review]: Kind of a kludge but seem like the protocol doesn't actually give us much choice. :) ::: src/idle-contact-info.c @@ +323,3 @@ + + if (request->contact_info == NULL) + I'm sure I've seen this code somewhere before. :) How about combining this with _is_valid_response? Something like this: static ContactInfoRequest * _is_valid_response (IdleConnection *conn, GValueArray *args) { // current body of _is_valid_response, returning NULL rather than FALSE; if (request->contact_info == NULL) request->contact_info = dbus_g_type_specialized_construct(TP_ARRAY_TYPE_CONTACT_INFO_FIELD_LIST); return request; } and then the boilerplate at the top of each function would just be: { // ... ContactInfoRequest *request = _is_valid_response (conn, args); if (request == NULL) return IDLE_PARSER_HANDLER_RESULT_NOT_HANDLED; // ... (This is a general comment on the contact-info code, rather than specifically about this patch.) Review of attachment 46405 [details] [review]: I don't blame you for your fantasies of bludgeoning ircd hackers. ::: src/idle-contact-info.c @@ +373,3 @@ + if (g_strcmp0(msg, "is logged in as")) + return IDLE_PARSER_HANDLER_RESULT_NOT_HANDLED; + So the message format is "<nickname> :is logged in as"? Weird! :) (In reply to comment #54) > Review of attachment 46405 [details] [review]: > > I don't blame you for your fantasies of bludgeoning ircd hackers. They get away with all these weird things because most IRC clients just dump whatever they get on the UI with minimal processing. Things get a bit tricky for Telepathy because stuff needs to fit into a pre-defined format. > ::: src/idle-contact-info.c > @@ +373,3 @@ > + if (g_strcmp0(msg, "is logged in as")) > + return IDLE_PARSER_HANDLER_RESULT_NOT_HANDLED; > + > > So the message format is "<nickname> :is logged in as"? Weird! :) Something like this: lindbohm.freenode.net 330 rishi debarshi :is logged in as Here I am using 'rishi' as my nick, but 'debarshi' is my primary nick (or account name) on Freenode. Weird but I guess it makes sense from the “just dump it to the terminal” viewpoint! ++ Committed and pushed. Created attachment 46501 [details] [review] Reduce boilerplate Created attachment 46523 [details] [review] Handle RPL_WHOISREGNICK messages in response to RequestContactInfo Created attachment 46524 [details] [review] Handle RPL_WHOISOPERATOR messages in response to RequestContactInfo Created attachment 46525 [details] [review] Handle RPL_WHOISSECURE messages in response to RequestContactInfo Review of attachment 46501 [details] [review]: Looks good. Review of attachment 46523 [details] [review]: Looks fine. Review of attachment 46524 [details] [review]: ++ Review of attachment 46525 [details] [review]: ++ Committed and pushed. Created attachment 46610 [details] [review] Put the role played by the target nick in a channel as vCard parameter Contains slight duplication of code from src/idle-handles.c. Review of attachment 46610 [details] [review]: ::: src/idle-contact-info.c @@ +63,3 @@ +{ + return (c == '#') || (c == '!') || (c == '&') || (c == '+'); +} Why not turn this code into a function in muc-channel, like _is_modechar(), and call it from the two places? @@ +304,3 @@ for (i = 0; channelsv[i] != NULL; i++) { + const gchar *channel = channelsv[i]; + const gchar *field_params[2] = {NULL, NULL}; This shouldn't be const. You shouldn't have to cast the const away at the end of the block when you free the string. I think you can past a char *[] to a function expecting const char * const *? It might be nice to avoid allocating a fresh string but I see why you have. :) (In reply to comment #68) > Review of attachment 46610 [details] [review]: > > ::: src/idle-contact-info.c > @@ +63,3 @@ > +{ > + return (c == '#') || (c == '!') || (c == '&') || (c == '+'); > +} > > Why not turn this code into a function in muc-channel, like _is_modechar(), > and call it from the two places? Done. > @@ +304,3 @@ > for (i = 0; channelsv[i] != NULL; i++) { > + const gchar *channel = channelsv[i]; > + const gchar *field_params[2] = {NULL, NULL}; > > This shouldn't be const. You shouldn't have to cast the const away at the end > of the block when you free the string. I think you can past a char *[] to a > function expecting const char * const *? Since we can't assign char ** to const char **, lets use a char ** and use the cast while passing the value to the function. Created attachment 46612 [details] [review] contactinfo-request.py: check the x-irc-server and x-host fields Created attachment 46613 [details] [review] Add idle_muc_channel_is_typechar as per RFC 2811 Yes, RFC 2811 and not 2812. :-) Created attachment 46614 [details] [review] Use idle_muc_channel_is_typechar Created attachment 46615 [details] [review] Put the role played by the target nick in a channel as vCard parameter Review of attachment 46612 [details] [review]: ++ Review of attachment 46613 [details] [review]: ++ Review of attachment 46614 [details] [review]: ++ Review of attachment 46615 [details] [review]: ++ Committed and pushed. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.