GNOME Bugzilla – Bug 708871
Port Folks to telepathy next
Last modified: 2018-09-21 16:04:47 UTC
Telepathy 1.0 aka "next" is an API break release, we need to port Folks telepathy backend to it. Branch: http://cgit.collabora.com/git/user/xclaesse/folks.git/log/?h=next
Created attachment 255940 [details] [review] Tests: Do no hardcode individual sha1 With telepathy 1.0 the account prefix is going to change, so all sha1 will have to be recalculated. It's better to calculate them so changing one line is enough to port to tp1.
Created attachment 255941 [details] [review] Update telepathy test utilities from telepathy-glib 'next' branch This commit won't build, it needs to be squashed with upcoming commits but is separated for now to make review easier. This is verbatim copy of telepathy-glib/tests/lib directory, no modification made.
Created attachment 255942 [details] [review] Port to telepathy-glib-1 Note the evil hack in Makefiles when generating vapi files. I have to use sed to change the namespace from "Tp." to "TelepathyGLib.". It seems vapigen is confused between telepathy-glib's gir namespace and its vapi namespace. I don't know why.
Review of attachment 255942 [details] [review]: A quick skim-read: ::: backends/telepathy/lib/tp-lowlevel.c @@ +68,3 @@ + tp_cli_dbus_properties_call_get (conn, -1, + TP_IFACE_CONNECTION_INTERFACE_ALIASING, "AliasFlags", + connection_get_alias_flags_cb, result, NULL, G_OBJECT (conn)); I'm sad that TpConnection doesn't do this, but we can fix that in telepathy-glib orthogonally. ::: backends/telepathy/lib/tpf-persona-store.vala @@ +396,3 @@ /* We do not trust local-xmpp or IRC at all, since Persona UIDs can be * faked by just changing hostname/username or nickname. */ + if (account.get_protocol_name () == "local-xmpp" || local_xmpp as of 0.99.2 @@ -763,3 @@ - if (!this.account.connection.has_interface_by_id ( - iface_quark_connection_interface_contact_list ())) Don't you want to keep this check so you can deal with CMs that genuinely have no contact list, like Idle? (It should be done by checking whether the appropriate feature is present rather than rummaging in D-Bus interfaces, though - that way, it isn't telepathy-glib-1-dbus.) ::: folks/folks-uninstalled.pc.in @@ +8,3 @@ Description: The Folks meta-contacts library Version: @VERSION@ +Requires: glib-2.0 gobject-2.0 gee-0.8 telepathy-glib-1 >= 0.99.1 Looks as though it also Requires.private telepathy-glib-1-dbus
(In reply to comment #4) > Review of attachment 255942 [details] [review]: > > A quick skim-read: > > ::: backends/telepathy/lib/tp-lowlevel.c > @@ +68,3 @@ > + tp_cli_dbus_properties_call_get (conn, -1, > + TP_IFACE_CONNECTION_INTERFACE_ALIASING, "AliasFlags", > + connection_get_alias_flags_cb, result, NULL, G_OBJECT (conn)); > > I'm sad that TpConnection doesn't do this, but we can fix that in > telepathy-glib orthogonally. Oh! actually Guillaume did it already: https://bugs.freedesktop.org/show_bug.cgi?id=28037 ! tl;dr but IIRC you were not happy with my branch setting alias because we wanted to move to Names first. So we can replace the alias flags with tp_connection_can_set_contact_alias(). > ::: backends/telepathy/lib/tpf-persona-store.vala > @@ +396,3 @@ > /* We do not trust local-xmpp or IRC at all, since Persona UIDs can be > * faked by just changing hostname/username or nickname. */ > + if (account.get_protocol_name () == "local-xmpp" || > > local_xmpp as of 0.99.2 Right, fixed. > @@ -763,3 @@ > > - if (!this.account.connection.has_interface_by_id ( > - iface_quark_connection_interface_contact_list ())) > > Don't you want to keep this check so you can deal with CMs that genuinely have > no contact list, like Idle? > > (It should be done by checking whether the appropriate feature is present > rather than rummaging in D-Bus interfaces, though - that way, it isn't > telepathy-glib-1-dbus.) good point, fixed. > ::: folks/folks-uninstalled.pc.in > @@ +8,3 @@ > Description: The Folks meta-contacts library > Version: @VERSION@ > +Requires: glib-2.0 gobject-2.0 gee-0.8 telepathy-glib-1 >= 0.99.1 > > Looks as though it also Requires.private telepathy-glib-1-dbus I still did not understand Requires VS Requires.private :-(
Review of attachment 255940 [details] [review]: As you said, this doesn't strictly require tp1, so please apply this now since it looks good (and doesn't break any tests). Just wrap it to 80 characters.
Review of attachment 255942 [details] [review]: Marking as needs-work as per Simon’s comments in comment #4. ::: backends/telepathy/lib/Makefile.am @@ +80,3 @@ --pkg gio-2.0 \ + --pkg telepathy-glib-1 \ + TpLowlevel-$(API_VERSION_DOT).gir && sed -i "s/Tp\./TelepathyGLib\./g" $@ Have you filed a bug against vapigen to get this fixed? It would be nice to be able to remove the sed in future (and so other users of the Telepathy VAPI don’t experience the same problem). What exactly is the problem? ::: backends/telepathy/tp-backend.vala @@ +86,3 @@ /* First handle adding any missing persona stores. */ GLib.List<unowned Account> accounts = + this._account_manager.dup_usable_accounts (); Given that this is now a ‘dup_*’ method, should the variable still be unowned? In any case, since it’s a GList I’d want to double-check the generated C is OK re. memory management. @@ +176,2 @@ GLib.List<unowned Account> accounts = + this._account_manager.dup_usable_accounts (); And here. Also, this is missing 4 spaces of indentation.
Comment on attachment 255941 [details] [review] Update telepathy test utilities from telepathy-glib 'next' branch This can be committed as soon as everything else is ready.
Comment on attachment 255940 [details] [review] Tests: Do no hardcode individual sha1 Committed a while ago. commit 47c63fd926cf1c6a6a0061e57662d3d3df1adc46 Author: Xavier Claessens <xavier.claessens@collabora.co.uk> Date: Thu Sep 26 16:06:41 2013 -0400 Tests: Do no hardcode individual sha1 With telepathy 1.0 the account prefix is going to change, so all sha1 will have to be recalculated. It's better to calculate them so changing one line is enough to port to tp1. tests/folks/aggregation.vala | 42 +++++++++----------------- tests/telepathy/individual-properties.vala | 11 +++++-- tests/telepathy/individual-retrieval.vala | 47 ++++++++++++++---------------- 3 files changed, 43 insertions(+), 57 deletions(-)
*** Bug 710805 has been marked as a duplicate of this bug. ***
As Simon mentioned in bug #710805, libfolks-telepathy (but not libfolks itself) needs a soname bump due to Telepathy undergoing a soname bump, and libfolks-telepathy exposing TpAccount and TpContact in its public API. That needs to be put into the next iteration of the patch, which will require adding build system machinery to support different LT versions for the different backends (currently they all share the same LT version as libfolks).
When fd.o cgit catches up, here are my latest changes to make Folks functional with 0.99.8: http://cgit.freedesktop.org/~smcv/folks/log/?h=next-0998 (Only tested via regression tests so far, not "live")
... and to move it to a version of telepathy-glib that replaces all dbus-glib connections with GDBus connections (currently unmerged): http://cgit.freedesktop.org/~smcv/folks/log/?h=next-gdbus (See <https://bugs.freedesktop.org/show_bug.cgi?id=28782>)
(In reply to comment #12) > When fd.o cgit catches up, here are my latest changes to make Folks functional > with 0.99.8: > > http://cgit.freedesktop.org/~smcv/folks/log/?h=next-0998 Reviewed-by: Xavier, pushed to 'next'.
Created attachment 273090 [details] [review] Fix TelepathyGLib GIR ABI version --- We don't want to link against both Telepathy0 and Telepathy1.
Created attachment 273091 [details] [review] libfolks-telepathy contains tp-lowlevel.c, so it needs -dbus CFLAGS/LIBS
Review of attachment 273091 [details] [review]: This seems safe to apply to master. Unless you have a reason to avoid doing so, please apply.
I pushed 720214d..20573e1 unreviewed, because it's minimal/necessary for current telepathy-glib. Xavier suggested that doing review for this sort of thing later would be less of a bottleneck; I'll make notes of unreviewed commits here.
a2cc576 also unreviewed, for telepathy-glib 0.99.11 (which I intend to release soon).
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/folks/issues/67.