Bug 43223

Branch at URL attempts to fix this.

I searched for all TODO/FIXMEs there and fixed those that requires fixing during the API/ABI break. The ones remaining can be fixed later if/when appropriate.
Also all deprecated signals/methods were removed.
Will update NEWS when merging this.

(In reply to comment #1)
> Branch at URL attempts to fix this.
> 
> I searched for all TODO/FIXMEs there and fixed those that requires fixing
> during the API/ABI break. The ones remaining can be fixed later if/when
> appropriate.

So it appears.

> Also all deprecated signals/methods were removed.
> Will update NEWS when merging this.

Good!

Some observations

 AbstractClient::AbstractClient()
+    : mPriv(new Private())
 {
 }

I see no corresponding delete, so this is leaked. AbstractClient dtor?

> Channel: Add includeSelfContact param to groupSelfContact and groupLocal/RemotePendingContacts.
 
Commit message clearly refers wrongly to groupSelfContact(). Also, docs missing for the includeSelfContact params.

+    if (!includeSelfContact) {
+        return mPriv->groupRemotePendingContacts.values().toSet() - Contacts() << groupSelfContact();
+    }
     return mPriv->groupRemotePendingContacts.values().toSet();

Something like

Contacts c = mPriv->whicheverContactsYoureReturning.values().toSet();
if (!includeSelfContact) {
  c.remove(groupSelfContact());
}
return c;

would be both cleaner (no duplicate mention of the source container) and faster (no singleton set created and trivially iterated for the self contact).

Otherwise looks good to me. I didn't look in Jeremy's big patches in detail, just an overview; I trust you've already done that when inheriting the work.

Adding dependency because the tp-qt (instead of tp-qt4) naming here depends on the qt5 branch.

(In reply to comment #2)
>  AbstractClient::AbstractClient()
> +    : mPriv(new Private())
>  {
>  }
> 
> I see no corresponding delete, so this is leaked. AbstractClient dtor?
Done.

> > Channel: Add includeSelfContact param to groupSelfContact and groupLocal/RemotePendingContacts.
> 
> Commit message clearly refers wrongly to groupSelfContact(). Also, docs missing
> for the includeSelfContact params.
> 
> +    if (!includeSelfContact) {
> +        return mPriv->groupRemotePendingContacts.values().toSet() - Contacts()
> << groupSelfContact();
> +    }
>      return mPriv->groupRemotePendingContacts.values().toSet();
> 
> Something like
> 
> Contacts c = mPriv->whicheverContactsYoureReturning.values().toSet();
> if (!includeSelfContact) {
>   c.remove(groupSelfContact());
> }
> return c;
> 
> would be both cleaner (no duplicate mention of the source container) and faster
> (no singleton set created and trivially iterated for the self contact).
Done.
 
> Otherwise looks good to me. I didn't look in Jeremy's big patches in detail,
> just an overview; I trust you've already done that when inheriting the work.
I did :)

+ * \param includeSelfContact Whether to include the self contact to the returned set.

"in the returned set"

I'd also explain it a bit more in the long form description of the methods. Because "self contact" is not really a common knowledge term.

Something like

* It is possible to omit the contact representing the local user, even if he/she * is in the set, by passing \c false as the parameter \a includeSelfContact.

Otherwise good to go after merging the qt5 branch.

(In reply to comment #4)
> + * \param includeSelfContact Whether to include the self contact to the
> returned set.
> 
> "in the returned set"
> 
> I'd also explain it a bit more in the long form description of the methods.
> Because "self contact" is not really a common knowledge term.
> 
> Something like
> 
> * It is possible to omit the contact representing the local user, even if
> he/she * is in the set, by passing \c false as the parameter \a
> includeSelfContact.
> 
Done

> Otherwise good to go after merging the qt5 branch.
Thanks, will merge.

Changes merged upstream. It will be in tp-qt 0.9.0