[Bug 27669] Implement Location interface support

bugzilla-daemon at freedesktop.org bugzilla-daemon at freedesktop.org
Wed Apr 21 17:12:58 CEST 2010


https://bugs.freedesktop.org/show_bug.cgi?id=27669

--- Comment #5 from Andre Moreira Magalhaes <andrunko at gmail.com> 2010-04-21 08:12:57 PDT ---
(In reply to comment #4)
> (In reply to comment #1)
> > Please separate this into two branches
> 
> Or three branches. That works too :-)
> 
> Basic codegen
> =============
> 
> review+ for andrunko/location.
> 
> Other people's locations
> ========================
> 
> > > +    if (!mPriv->requestedFeatures.contains(FeatureLocation)) {
> > 
> > Is requestedFeatures really the right member there?
> 
> I still think this is wrong.
No, it is not

> 
> > > -QVariantMap Contact::location() const
> > > +ContactLocation *Contact::location() const
> > 
> > This seems to return a pointer that's only valid as long as the Contact is. Is
> > that appropriate? It should at least be documented.
> > 
> > It should probably either be a const pointer that's documented to have the same
> > validity as the Contact, or a refcounted Tp::ContactLocationPtr?
> 
> I still think this is wrong, and I'd be inclined to define and use a
> Tp::ContactLocationPtr (or you could turn ContactLocation into a value type
> with a refcounted private struct, like QString, if you prefer).
I wouldn't go this route. It's normal in Qt do this also as returning a
QObject* for example. We also decided to go this route for ContactCapabilities
so I believe we should follow what we already have.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.



More information about the telepathy-bugs mailing list