- Connection doesn't claim to support MailNotifications even when it does - Protocol['ConnectionInterfaces'] could be smarter and return the interfaces actually implemented for this protocol
Created attachment 85977 [details] [review] connection: factor out protocol_info_supports_{avatar,blocking}
Created attachment 85978 [details] [review] connection: claim to support MailNotification if supported
Created attachment 85979 [details] [review] connection: factor out add_optionnal_connection_interfaces()
Created attachment 85980 [details] [review] factor out add_always_present_connection_interfaces()
Created attachment 85981 [details] [review] protocol: fix 'ConnectionInterfaces'
Comment on attachment 85977 [details] [review] connection: factor out protocol_info_supports_{avatar,blocking} Review of attachment 85977 [details] [review]: ----------------------------------------------------------------- ++
Comment on attachment 85978 [details] [review] connection: claim to support MailNotification if supported Review of attachment 85978 [details] [review]: ----------------------------------------------------------------- Yes (but also, this interface was standardized - we should implement the standard version)
Comment on attachment 85979 [details] [review] connection: factor out add_optionnal_connection_interfaces() Review of attachment 85979 [details] [review]: ----------------------------------------------------------------- r+ with a spelling fix ::: src/connection.c @@ +174,4 @@ > } > > static void > +add_optionnal_connection_interfaces (GPtrArray *ifaces, "optional"
Comment on attachment 85980 [details] [review] factor out add_always_present_connection_interfaces() Review of attachment 85980 [details] [review]: ----------------------------------------------------------------- ++
Comment on attachment 85981 [details] [review] protocol: fix 'ConnectionInterfaces' Review of attachment 85981 [details] [review]: ----------------------------------------------------------------- ::: src/connection.c @@ +140,5 @@ > + > + ifaces = g_ptr_array_new (); > + add_always_present_connection_interfaces (ifaces); > + add_optionnal_connection_interfaces (ifaces, prpl_info); > + g_ptr_array_add (ifaces, NULL); Please either comment that it returns an array with NULL at the end, or just return a (transfer full) gchar **. I think I'd prefer the latter.
(In reply to comment #7) > Comment on attachment 85978 [details] [review] [review] > connection: claim to support MailNotification if supported > > Review of attachment 85978 [details] [review] [review]: > ----------------------------------------------------------------- > > Yes (but also, this interface was standardized - we should implement the > standard version) I opened bug #69468. (In reply to comment #8) > Comment on attachment 85979 [details] [review] [review] > connection: factor out add_optionnal_connection_interfaces() > > Review of attachment 85979 [details] [review] [review]: > ----------------------------------------------------------------- > > r+ with a spelling fix > > ::: src/connection.c > @@ +174,4 @@ > > } > > > > static void > > +add_optionnal_connection_interfaces (GPtrArray *ifaces, > > "optional" fixed.
Created attachment 85986 [details] [review] protocol: fix 'ConnectionInterfaces'
(In reply to comment #10) > Comment on attachment 85981 [details] [review] [review] > protocol: fix 'ConnectionInterfaces' > > Review of attachment 85981 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: src/connection.c > @@ +140,5 @@ > > + > > + ifaces = g_ptr_array_new (); > > + add_always_present_connection_interfaces (ifaces); > > + add_optionnal_connection_interfaces (ifaces, prpl_info); > > + g_ptr_array_add (ifaces, NULL); > > Please either comment that it returns an array with NULL at the end, or just > return a (transfer full) gchar **. I think I'd prefer the latter. Humm actually "(transfer full) gchar **" is annoying because TpBaseConnection->get_interfaces_always_present is supposed to return a (transfer container) GPtrArray while TpBaseProtocol->get_connection_details returns a (transfer full) gchar **. I'd like to reuse the same function for both so I'll go with the former option.
Created attachment 85991 [details] [review] protocol: fix 'ConnectionInterfaces'
(In reply to comment #13) > Humm actually "(transfer full) gchar **" is annoying because > TpBaseConnection->get_interfaces_always_present is supposed to return a > (transfer container) GPtrArray while TpBaseProtocol->get_connection_details > returns a (transfer full) gchar **. g_i_a_p isn't meant to have the NULL at the end, though? So you'll have to have a conversion step either way, even if it's as simple as "copy each string, and append NULL".
I wish GPtrArray could handle the terminal NULL itself: https://bugzilla.gnome.org/show_bug.cgi?id=631210
(In reply to comment #15) > (In reply to comment #13) > > Humm actually "(transfer full) gchar **" is annoying because > > TpBaseConnection->get_interfaces_always_present is supposed to return a > > (transfer container) GPtrArray while TpBaseProtocol->get_connection_details > > returns a (transfer full) gchar **. > > g_i_a_p isn't meant to have the NULL at the end, though? So you'll have to > have a conversion step either way, even if it's as simple as "copy each > string, and append NULL". You're right: - TpBaseProtocolGetConnectionDetailsFunc returns a transfer full NULL terminated gchar **. - get_interfaces_always_present returns a transfer container not-NULL terminated GPtrArray. haze_connection_dup_implemented_interfaces now returns (transfer container) and I dup the strings and append NULL afterward.
Created attachment 86039 [details] [review] 85991: protocol: fix 'ConnectionInterfaces'
Comment on attachment 86039 [details] [review] 85991: protocol: fix 'ConnectionInterfaces' Review of attachment 86039 [details] [review]: ----------------------------------------------------------------- ::: src/protocol.c @@ +910,5 @@ > + ifaces = haze_connection_dup_implemented_interfaces ( > + self->priv->prpl_info); > + /* @connection_interfaces takes a NULL terminated (transfer full) > + * gchar ** so we have to dup each string and append NULL. */ > + g_ptr_array_foreach (ifaces, (GFunc) g_strdup, NULL); No, this copies the string, throws away the copy, and leaves the original in the array. Use a "for" loop.
Created attachment 86059 [details] [review] 86039: 85991: protocol: fix 'ConnectionInterfaces' I blame me waking up at 7am for the previous patch. :)
Comment on attachment 86059 [details] [review] 86039: 85991: protocol: fix 'ConnectionInterfaces' Review of attachment 86059 [details] [review]: ----------------------------------------------------------------- better!
Merged to master.
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.