Problem with appletRemoved signal

[Alexis Ménard]

Hello folks,


There is a problem with appletRemoved public signal in containment. Here is
the signature :

/**
 * This signal is emitted when an applet is destroyed
 */
 void appletRemoved(Plasma::Applet *applet);

So a containment that is connected to this signal expect an Applet pointer.
But who emit this signal? Our beloved base class Containment which is
connected to the QObject destroyed signal of the applet.

Here is the slot :
void ContainmentPrivate::appletDestroyed(QObject *object);

When we arrive in the this slot applet destructor has been already called.
Since we do a static_cast just because we need the value of the pointer,
it's ok. BUT after we call :

emit q->appletRemoved(applet); with the pointer we get after the
static_cast.

Aie aie aie. Valgrind complain and we never know, people can use the pointer
to do some stuff when they get the appletRemoved signal, (we already do that
in our panel.cpp class). Luckily it doesn't crash but it can happen.

I have attach a patch that create a appletDestroyed signal that is emit just
after entering in Applet destructor. I wanted to use destroyed but we have
already a method called like this.

I guess if people agreed that i should backport it to 4.2 branch.

Is it OK?

PS : Sorry for don't using the review board but it doesn't work with git, i
don't have the svn revision.

diff --git a/plasma/applet.cpp b/plasma/applet.cpp
index 0956fdf..b4116af 100644
--- a/plasma/applet.cpp
+++ b/plasma/applet.cpp
@@ -135,6 +135,9 @@ Applet::Applet(QObject *parentObject, const QVariantList &args)
 
 Applet::~Applet()
 {
+    //let people know that i will die
+    emit appletDestroyed(this);
+
     if (!d->transient && d->extender) {
         //This would probably be nicer if it was located in extender. But in it's dtor, this won't
         //work since when that get's called, the applet's config() isn't accessible anymore. (same
diff --git a/plasma/applet.h b/plasma/applet.h
index 5f4ad71..9d5324d 100644
--- a/plasma/applet.h
+++ b/plasma/applet.h
@@ -611,6 +611,11 @@ class PLASMA_EXPORT Applet : public QGraphicsWidget
          */
         void activate();
 
+        /**
+         * Emitted when the applet is deleted
+         */
+        void appletDestroyed(Plasma::Applet *applet);
+
     public Q_SLOTS:
         /**
          * Sets the immutability type for this applet (not immutable,
diff --git a/plasma/containment.cpp b/plasma/containment.cpp
index 9cc469b..73d5a7b 100644
--- a/plasma/containment.cpp
+++ b/plasma/containment.cpp
@@ -735,7 +735,7 @@ void Containment::addApplet(Applet *applet, const QPointF &pos, bool delayInit)
 
     connect(applet, SIGNAL(configNeedsSaving()), this, SIGNAL(configNeedsSaving()));
     connect(applet, SIGNAL(releaseVisualFocus()), this, SIGNAL(releaseVisualFocus()));
-    connect(applet, SIGNAL(destroyed(QObject*)), this, SLOT(appletDestroyed(QObject*)));
+    connect(applet, SIGNAL(appletDestroyed(Plasma::Applet*)), this, SLOT(appletDestroyed(Plasma::Applet*)));
     connect(applet, SIGNAL(activate()), this, SIGNAL(activate()));
 
     if (pos != QPointF(-1, -1)) {
@@ -1698,16 +1698,8 @@ bool ContainmentPrivate::regionIsEmpty(const QRectF &region, Applet *ignoredAppl
     return true;
 }
 
-void ContainmentPrivate::appletDestroyed(QObject *object)
+void ContainmentPrivate::appletDestroyed(Plasma::Applet *applet)
 {
-    // we do a static_cast here since it really isn't an Applet by this
-    // point anymore since we are in the qobject dtor. we don't actually
-    // try and do anything with it, we just need the value of the pointer
-    // so this unsafe looking code is actually just fine.
-    //
-    // NOTE: DO NOT USE THE applet VARIABLE FOR ANYTHING OTHER THAN COMPARING
-    //       THE ADDRESS! ACTUALLY USING THE OBJECT WILL RESULT IN A CRASH!!!
-    Applet *applet = static_cast<Plasma::Applet*>(object);
     applets.removeAll(applet);
     if (focusedApplet == applet) {
         focusedApplet = 0;
diff --git a/plasma/containment.h b/plasma/containment.h
index 354ccb7..e234341 100644
--- a/plasma/containment.h
+++ b/plasma/containment.h
@@ -507,7 +507,7 @@ class PLASMA_EXPORT Containment : public Applet
         const QGraphicsItem *toolBoxItem() const;
 
     private:
-        Q_PRIVATE_SLOT(d, void appletDestroyed(QObject*))
+        Q_PRIVATE_SLOT(d, void appletDestroyed(Plasma::Applet*))
         Q_PRIVATE_SLOT(d, void containmentAppletAnimationComplete(QGraphicsItem *item,
                                                                   Plasma::Animator::Animation anim))
         Q_PRIVATE_SLOT(d, void triggerShowAddWidgets())
diff --git a/plasma/private/containment_p.h b/plasma/private/containment_p.h
index 6fe21b7..0a91b98 100644
--- a/plasma/private/containment_p.h
+++ b/plasma/private/containment_p.h
@@ -75,7 +75,7 @@ public:
     void positionContainments();
     void setLockToolText();
     void handleDisappeared(AppletHandle *handle);
-    void appletDestroyed(QObject*);
+    void appletDestroyed(Plasma::Applet*);
     void containmentAppletAnimationComplete(QGraphicsItem *item, Plasma::Animator::Animation anim);
     void zoomIn();
     void zoomOut();

_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel