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

[Andre Moreira Magalhaes]

-------- Original Message --------
Subject: 	tp-qt4 qsharedpointer usage
Date: 	Wed, 18 Mar 2009 01:24:37 -0300
From: 	Andre Moreira Magalhaes <andre.magalhaes at collabora.co.uk>
To: 	Simon McVittie <simon.mcvittie at collabora.co.uk>, Olli Salli 
<olli.salli at collabora.co.uk>



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?

Regards
Andre