After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 708871 - Port Folks to telepathy next
Port Folks to telepathy next
Status: RESOLVED OBSOLETE
Product: folks
Classification: Platform
Component: general
git master
Other Linux
: Normal normal
: 0.12.x
Assigned To: folks-maint
folks-maint
: 710805 (view as bug list)
Depends on: 696177
Blocks:
 
 
Reported: 2013-09-26 20:19 UTC by Xavier Claessens
Modified: 2018-09-21 16:04 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Tests: Do no hardcode individual sha1 (9.14 KB, patch)
2013-09-27 14:45 UTC, Xavier Claessens
committed Details | Review
Update telepathy test utilities from telepathy-glib 'next' branch (311.35 KB, patch)
2013-09-27 14:46 UTC, Xavier Claessens
accepted-commit_now Details | Review
Port to telepathy-glib-1 (23.57 KB, patch)
2013-09-27 14:46 UTC, Xavier Claessens
needs-work Details | Review
Fix TelepathyGLib GIR ABI version (1.88 KB, patch)
2014-03-27 13:49 UTC, Simon McVittie
none Details | Review
libfolks-telepathy contains tp-lowlevel.c, so it needs -dbus CFLAGS/LIBS (868 bytes, patch)
2014-03-27 13:49 UTC, Simon McVittie
accepted-commit_now Details | Review

Description Xavier Claessens 2013-09-26 20:19:37 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
Comment 1 Xavier Claessens 2013-09-27 14:45:58 UTC
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.
Comment 2 Xavier Claessens 2013-09-27 14:46:01 UTC
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.
Comment 3 Xavier Claessens 2013-09-27 14:46:04 UTC
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.
Comment 4 Simon McVittie 2013-09-27 17:19:22 UTC
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
Comment 5 Xavier Claessens 2013-09-27 19:40:04 UTC
(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 :-(
Comment 6 Travis Reitter 2013-09-27 21:31:47 UTC
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.
Comment 7 Philip Withnall 2013-09-28 13:03:05 UTC
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 8 Philip Withnall 2013-10-14 11:46:59 UTC
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 9 Philip Withnall 2013-10-14 16:55:58 UTC
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(-)
Comment 10 Philip Withnall 2013-10-24 16:22:11 UTC
*** Bug 710805 has been marked as a duplicate of this bug. ***
Comment 11 Philip Withnall 2013-10-24 16:24:49 UTC
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).
Comment 12 Simon McVittie 2014-03-25 12:18:22 UTC
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")
Comment 13 Simon McVittie 2014-03-25 12:46:22 UTC
... 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>)
Comment 14 Simon McVittie 2014-03-25 14:57:57 UTC
(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'.
Comment 15 Simon McVittie 2014-03-27 13:49:05 UTC
Created attachment 273090 [details] [review]
Fix TelepathyGLib GIR ABI version

---

We don't want to link against both Telepathy0 and Telepathy1.
Comment 16 Simon McVittie 2014-03-27 13:49:20 UTC
Created attachment 273091 [details] [review]
libfolks-telepathy contains tp-lowlevel.c, so it needs -dbus  CFLAGS/LIBS
Comment 17 Travis Reitter 2014-03-27 22:49:14 UTC
Review of attachment 273091 [details] [review]:

This seems safe to apply to master. Unless you have a reason to avoid doing so, please apply.
Comment 18 Simon McVittie 2014-04-08 19:23:11 UTC
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.
Comment 19 Simon McVittie 2014-05-07 09:53:12 UTC
a2cc576 also unreviewed, for telepathy-glib 0.99.11 (which I intend to release soon).
Comment 20 GNOME Infrastructure Team 2018-09-21 16:04:47 UTC
-- 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.