Bug 43223 - Resolve API/ABI break TODO issues before 0.9.0 (qt4/qt5) release
Summary: Resolve API/ABI break TODO issues before 0.9.0 (qt4/qt5) release
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: tp-qt (show other bugs)
Version: git master
Hardware: Other All
: high enhancement
Assignee: Andre Moreira Magalhaes
QA Contact: Telepathy bugs list
URL: http://cgit.collabora.com/git/user/an...
Whiteboard: review+
Keywords: patch
Depends on: 43222
Blocks: 43240
  Show dependency treegraph
 
Reported: 2011-11-24 07:15 UTC by Olli Salli
Modified: 2011-12-06 05:22 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments

Description Olli Salli 2011-11-24 07:15:32 UTC
There are quite a few even with the current state of the mix-and-match qt5 megabranch for bug 43222.

The notes can be found most easily by case insensitive grep for todo|fixme. These are things which we haven't been able to mark with the DEPRECATED macro, as that's only applicable to methods and types to be removed, or the runtime signal deprecation system.
Comment 1 Andre Moreira Magalhaes 2011-11-24 14:00:37 UTC
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.
Comment 2 Olli Salli 2011-11-25 04:04:12 UTC
(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.
Comment 3 Andre Moreira Magalhaes 2011-11-25 07:07:59 UTC
(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 :)
Comment 4 Olli Salli 2011-11-25 07:21:18 UTC
+ * \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.
Comment 5 Andre Moreira Magalhaes 2011-11-25 07:37:06 UTC
(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.
Comment 6 Andre Moreira Magalhaes 2011-12-06 05:22:45 UTC
Changes merged upstream. It will be in tp-qt 0.9.0


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.