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 658542 - Use new Subject and RoomConfig APIs rather than Telepathy.Properties
Use new Subject and RoomConfig APIs rather than Telepathy.Properties
Status: RESOLVED FIXED
Product: empathy
Classification: Core
Component: Multi User Chat
3.1.x
Other Linux
: Normal normal
: ---
Assigned To: empathy-maint
Depends on:
Blocks:
 
 
Reported: 2011-09-08 11:24 UTC by Will Thompson
Modified: 2011-10-14 17:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
TpChat: hide guts of Telepathy properties. (11.97 KB, patch)
2011-09-08 11:27 UTC, Will Thompson
committed Details | Review
TpChat: track subject and title explicitly. (4.82 KB, patch)
2011-09-08 11:27 UTC, Will Thompson
committed Details | Review
TpChat: Use RoomConfig1 and Subject if available (5.07 KB, patch)
2011-09-08 11:27 UTC, Will Thompson
none Details | Review
TpChat: remove support for old Properties (9.81 KB, patch)
2011-09-08 11:27 UTC, Will Thompson
committed Details | Review
TpChat: Use RoomConfig1 and Subject if available (5.03 KB, patch)
2011-09-13 14:16 UTC, Will Thompson
none Details | Review
TpChat: Use RoomConfig1 and Subject if available (5.37 KB, patch)
2011-09-13 15:55 UTC, Will Thompson
none Details | Review
TpChat: Use RoomConfig1 and Subject if available (5.04 KB, patch)
2011-09-13 15:56 UTC, Will Thompson
committed Details | Review

Description Will Thompson 2011-09-08 11:24:29 UTC
Hello.

I've been working on getting rid of Telepathy properties, catalyzed by an issue where change notification for them is broken, largely unfixably.

Here is an Empathy branch which switches to using the Subject and RoomConfig APIs. It depends on the spec (and a corresponding tp-glib branch) from <https://bugs.freedesktop.org/show_bug.cgi?id=23151>.

http://cgit.collabora.com/git/user/wjt/empathy.git/log/?h=subjective
Comment 1 Will Thompson 2011-09-08 11:27:16 UTC
Created attachment 195970 [details] [review]
TpChat: hide guts of Telepathy properties.

This will make it easier to replace these with new stuff. The funky
indentation in the callbacks for the subject and title changing is to
make it clear that I didn't change that code; I'll reindent it in
another patch.
Comment 2 Will Thompson 2011-09-08 11:27:19 UTC
Created attachment 195971 [details] [review]
TpChat: track subject and title explicitly.

This will allow us to also use the new API (and, ultimately, delete
the Telepathy.Properties code cleanly).
Comment 3 Will Thompson 2011-09-08 11:27:21 UTC
Created attachment 195972 [details] [review]
TpChat: Use RoomConfig1 and Subject if available
Comment 4 Will Thompson 2011-09-08 11:27:24 UTC
Created attachment 195973 [details] [review]
TpChat: remove support for old Properties

Change notification for this is broken in the CMs *anyway*.
Comment 5 Xavier Claessens 2011-09-12 15:21:21 UTC
 - the tp_str_empty stuff in update_title: Why do you special-case title="" and force it to NULL but don't do the same for subject?

 - In update_title() you do g_hash_table_lookup and then tp_asv_get_string() which is 2 lookups where one is enough. Just do title = tp_asv_get_string (properties, "Title"); if (title!= NULL) {}

The rest seems all good.
Comment 6 Will Thompson 2011-09-13 14:02:40 UTC
(In reply to comment #5)
>  - the tp_str_empty stuff in update_title: Why do you special-case title="" and
> force it to NULL but don't do the same for subject?

Because the surrounding code assumes that if the title is non-NULL, then it's meaningful, whereas it handles an empty subject as equivalent to no subject. In short, this was to make these changes minimally-invasive.

If you like, I could do any of:

a) Add the same special-case to Subject;
b) Change the existing code that uses Title to check for empty;
c) Both;
d) Neither.

>  - In update_title() you do g_hash_table_lookup and then tp_asv_get_string()
> which is 2 lookups where one is enough. Just do title = tp_asv_get_string
> (properties, "Title"); if (title!= NULL) {}

Fair enough.
Comment 7 Xavier Claessens 2011-09-13 14:07:42 UTC
(In reply to comment #6)
> (In reply to comment #5)
> >  - the tp_str_empty stuff in update_title: Why do you special-case title="" and
> > force it to NULL but don't do the same for subject?
> 
> Because the surrounding code assumes that if the title is non-NULL, then it's
> meaningful, whereas it handles an empty subject as equivalent to no subject. In
> short, this was to make these changes minimally-invasive.

I don't care much tbh, just was wondering if there were a serious reason. Keeping minimally-invesive changes is enough for me :)
Comment 8 Will Thompson 2011-09-13 14:16:53 UTC
Created attachment 196381 [details] [review]
TpChat: Use RoomConfig1 and Subject if available
Comment 9 Xavier Claessens 2011-09-13 14:35:30 UTC
there is double lookup in update_subject() too.

why did you change to tp_str_empty() with *title == '\0' ? I personally prefer the tp_str_empty even if we already changed it's not NULL. But doesn't matter, as you wish :)
Comment 10 Will Thompson 2011-09-13 15:55:11 UTC
Created attachment 196402 [details] [review]
TpChat: Use RoomConfig1 and Subject if available
Comment 11 Will Thompson 2011-09-13 15:55:22 UTC
Comment on attachment 196381 [details] [review]
TpChat: Use RoomConfig1 and Subject if available

take three!
Comment 12 Will Thompson 2011-09-13 15:56:43 UTC
Created attachment 196403 [details] [review]
TpChat: Use RoomConfig1 and Subject if available
Comment 13 Xavier Claessens 2011-09-13 16:12:17 UTC
Looks all good now. You can merge once the related stuff landed into spec/tp-glib/gabble/idle :)
Comment 14 Will Thompson 2011-10-12 10:05:47 UTC
Given that subject using Tp properties is broken, I would like to merge this into Empathy 3.2 so that it works again. If bumping the tp-glib dependency is not an option in a point release (even to the to-be-released stable branch of tp-glib), I could swaddle the new code in version-checking guards.

Dear maintainers, which would you prefer?
Comment 15 Xavier Claessens 2011-10-12 13:15:23 UTC
Since it depends on 0.15.x I would say it's fine to bump until 0.16.0 tbh.
Comment 16 Guillaume Desmottes 2011-10-12 14:05:27 UTC
Agreed, but let's release tp-glib 0.16.0 so we depends on the stable branch.
Comment 17 Will Thompson 2011-10-14 16:16:18 UTC
I've updated the branch to have a patch depending on tp-glib 0.16.0 (and a patch for a crash I hit while testing).
Comment 18 Guillaume Desmottes 2011-10-14 17:03:43 UTC
Looks good; please merge to 3.2 and master.