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.

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).

Created attachment 195972 [details] [review]
TpChat: Use RoomConfig1 and Subject if available

Created attachment 195973 [details] [review]
TpChat: remove support for old Properties

Change notification for this is broken in the CMs *anyway*.

 - 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.

(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.

(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 :)

Created attachment 196381 [details] [review]
TpChat: Use RoomConfig1 and Subject if available

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 :)

Created attachment 196402 [details] [review]
TpChat: Use RoomConfig1 and Subject if available

Comment on attachment 196381 [details] [review]
TpChat: Use RoomConfig1 and Subject if available

take three!

Created attachment 196403 [details] [review]
TpChat: Use RoomConfig1 and Subject if available

Looks all good now. You can merge once the related stuff landed into spec/tp-glib/gabble/idle :)

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?

Since it depends on 0.15.x I would say it's fine to bump until 0.16.0 tbh.

Agreed, but let's release tp-glib 0.16.0 so we depends on the stable branch.

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).

Looks good; please merge to 3.2 and master.

Thanks, merged to gnome-3-2 <http://git.gnome.org/browse/empathy/log/?id=0bf6ee3a15d15cb80c5835d33bf899b2404da053> and master <http://git.gnome.org/browse/empathy/log/?id=68fc486>