[Telepathy] [Fwd: tp-qt4 qsharedpointer usage]

[Ian Monroe]

On Wednesday 18 March 2009 07:49:19 Andre Moreira Magalhaes wrote:
> Hi guys,
>
> While developing the call example, I found some issues regarding tp-qt4
> QSharedPointer usage that I would like to discuss.
>
> In the current version we are using QSharedPointer whenever we need to
> create a new object, letting the user handle the object lifetime. This
> is ok in some cases but it becomes a problem in others.
>
> The following code snippet demonstrate the problem:
>
> class CallHandler : public QObject
> {
>   ...
>
>   void addIncomingCall(const QSharedPointer<Channel> &chan);
>
> slots:
>   void onCHannelReady(PendingOperation *op);
>
> private:
>   QSharedPointer<Connection *> conn;
>   QList<QSharedPointer<Channel> > chans;
>   QList<CallWidget *> calls;
> }
>
> class CallWidget : public QWidget
> {
>   CallWidget(const QSharedPointer<Channel> &chan);
>   ...
> }
>
> void ChanHandler::addOutgoingCall(const QSharedPointer<Contact> &contact)
> {
>  ...
>  PendingChannel *pc = conn->ensureChannel(...);
>  connect(pc,
>          SIGNAL(finished(...)),
>          SLOT(onOutgoingChannelCreated(...)));
> }
>
> void ChanHandler::onOutgoingChannelCreated(PendingOperation *op)
> {
>  PendingChannel *pc = (PendingChannel *) op;
>
>  // we need to keep the shared pointer here or it will be
>  // delete when PendingOperation gets deleted.
>  chans.append(pc->channel());
>
>  connect(chan->becomeReady(),
>          SIGNAL(finished(...)),
>          SLOT(onChannelReady(...)));
> }
>
> void ChanHandler::addIncomingCall(const QSharedPointer<Channel> &chan)
> {
>  chans.append(chan);
>  connect(chan->becomeReady(),
>          SIGNAL(finished(...)),
>          SLOT(onChannelReady(...)));
> }
>
> void CHanHandler::onChannelReady(PendingOperation *op)
> {
>  PendingReady *pr = (PendingReady *) op;
>
>  // the chan is no more a shared pointer, so to check which
> QSharedPointer<Channel> became ready,
>  // we need to go trough the list of channels
>  Channel *chan = (Channel*) pr->object();
>
>  foreach (const QSharedPointer<Channel> &sharedChan, chans) {
>    if (sharedChan.data == chan) {
>      // create call widget for chan
>      calls.append(new CallWidget(sharedChan));
>    }
>  }
> }
>
> So basically, the problem is, we use QSharedPointer in some places, and
> in some we don't, basically because we can't. PendingReady::object as of
> now for example cannot return a QSharedPointer, as it does not know
> about the sharedpointer that first called becomeReady.
>
> This is just a basic example, while developing the call example, I came
> up with such situations in various places, and it's really annoying.
>
> Solutions:
> - Stop using QSharedPointer everywhere. Use it for contacts and some
> other really needed places and that's it. Let the user handle the
> objects lifetime,
> or pass the parent QObject to all methods that need to create a new
> object (PendingConnection::connection, PendingChannel::channel, ...)
>
> - Create our own TpSharedPtr class that is smarter than QSharedPointer
> and we can use everywhere (not sure if it's feasible, needs to think
> more about it). It could be just a weakref internally in places we don't
> want/need to delete the object PendingReady::object for example.
>
> - Have a ObjectsPool class that stores all shared pointers and have a
> method to retrieve a shared pointer by doing a lookup of the object that
> the shared pointer points to. It should keep a weakref of the shared
> pointer, removing it from the pool when it gets deleted.
>  We could even make PendingReady a template class and have it return a
> QSharedPointer<T> on the object method, this could be done by all
> methods that return an object *, so we could always return a
> QSharedPointer.
>
> Examples of places that do not return a QSharedPointer:
> - Contact::connectionManager
> - Connection::connection
> - PendingReady::object
> - Account::accountManager
> ...
>
> I am just coming with this now, as I am sure users will complain about
> it, it's really really annoying the way it's now, and I could only see
> this when I tried to develop a app that uses it.
> So if we come up with a good solution, it's better to do it now than
> later IMHO.
>
> What do you guys think?

I wrote a bug about the issue last month:
https://bugs.freedesktop.org/show_bug.cgi?id=20299

Like I said in the bug report, using KSharedPtr would allow us to always 
return a KSharedPtr since the counting occurs in the object itself (via 
QSharedPointer), instead of an external pointer type.

I think (but am not positive) that KSharedPtr is the same thing as 
QExplictlySharedDataPointer, just without a detach call. So we could use 
QExplictlySharedDataPointer and ignore detach.

Personally I think this kind of pointer always make more sense, having the 
counting occur in the pointer is asking for various types of trouble. What 
Andre outlined above seems inevitable.

Ian