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
_______________________________________________
telepathy mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/telepathy
