[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