Merge lp:~nick-dedekind/dee-qt/glib-events into lp:dee-qt

[Nick Dedekind]

Fixes the issue where calling into Qt directly from glib callbacks can cause deleteLater events not to serviced by qt event loop.

Send qt events from glib callbacks.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:85
http://jenkins.qa.ubuntu.com/job/dee-qt-ci/13/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/dee-qt-trusty-amd64-ci/1/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/dee-qt-trusty-armhf-ci/1/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/dee-qt-ci/13/rebuild

review: Needs Fixing (continuous-integration)

[Albert Astals Cid (aacid)]

The build failure is a regression of cmake 2.8.12 in trusty, the regression is fixed in cmake 2.8.12.1 that at the time of writing this comment is in trusty-proposed

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:85
http://jenkins.qa.ubuntu.com/job/dee-qt-ci/15/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dee-qt-trusty-amd64-ci/3
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dee-qt-trusty-armhf-ci/3

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/dee-qt-ci/15/rebuild

review: Approve (continuous-integration)

[Michal Hruby (mhr3)]

Instead of putting everything inside a lambda and increasing the indent inside the actual signal handlers, could we move the QGlibCallbackEvent into a lambda where the signals are connected? That way it's clearer why it's there plus the handler methods themselves could be cleaned up from the unused parameters (and made non-static).

So for example:
g_signal_connect_swapped(m_deeModel, "row-added",
  [] (DeeListModel* model, DeeModelIter* iter) {
    QGlibCallbackEvent::sendEvent(model, [model, iter] (QObject*) {
      model->d->onRowAdded(iter);
    }
  }, m_parent);

review: Needs Fixing

[Nick Dedekind (nick-dedekind)]

hmm. I'm getting a conversion error for the callback. You sure a lambda can be used in place of GCallback?

invalid user-defined conversion from ‘DeeListModelPrivate::connectToDeeModel(DeeModel*)::__lambda0’ to ‘GCallback {aka void (*)()}’ [-fpermissive]

[Michal Hruby (mhr3)]

Well, you still need to cast it with G_CALLBACK([] () {}), that should do it.

[Nick Dedekind (nick-dedekind)]

> Well, you still need to cast it with G_CALLBACK([] () {}), that should do it.

FAIL.

g_signal_connect(m_deeModel, "row-added", G_CALLBACK([](GObject *emitter,
                                                        DeeModelIter* iter,
                                                        DeeListModel* model) {
    QGlibCallbackEvent::sendEvent(model, [model, iter](QObject*) {
        model->d->onRowAdded(iter);
    });
}), m_parent);

[Nick Dedekind (nick-dedekind)]

Also, now that it's using an anonymous function, we can no longer disconnect from it. using g_object_disconnect. Would need to store the signal id and disconnect using g_signal_handler_disconnect for each connection.

[Michal Hruby (mhr3)]

> Also, now that it's using an anonymous function, we can no longer disconnect
> from it. using g_object_disconnect. Would need to store the signal id and
> disconnect using g_signal_handler_disconnect for each connection.

No biggie, but there's still g_signal_handlers_disconnect_by_data() if you didn't want to keep the signal ids.

[Michal Hruby (mhr3)]

> Well, you still need to cast it with G_CALLBACK([] () {}), that should do it.

Had hard time believing that it wouldn't work, but it doesn't indeed... Nonetheless with a cast in-between it does:

    typedef void(*DeeModelIterFunc)(DeeListModel*, DeeModelIter*);

    g_signal_connect_swapped(m_deeModel, "row-added", G_CALLBACK((DeeModelIterFunc)([] (DeeListModel* model, DeeModelIter* iter)
    {
        QGlibCallbackEvent::sendEvent(model, [model, iter](QObject*) {
            model->d->onRowAdded(iter);
        });
    })), m_parent);

[Michal Hruby (mhr3)]

Better yet:

#define LAMBDA_HANDLER(f) (GCallback) ((void(*)(DeeListModel*, ...)) (f))
g_signal_connect_swapped(m_deeModel, "row-added", LAMBDA_HANDLER([](..){..}), ..);

;)

Unmerged revisions

85. By Nick Dedekind on 2013-11-15

Use qevent to handle glibevents

=== modified file 'deelistmodel.cpp'
--- deelistmodel.cpp	2013-09-19 17:47:01 +0000
+++ deelistmodel.cpp	2013-11-18 11:20:05 +0000
@@ -25,6 +25,9 @@
 
 #include "deelistmodel.h"
 #include "variantconversions.h"
+#include "glibcallbackevent.h"
+
+const QEvent::Type QGlibCallbackEvent::eventType = static_cast<QEvent::Type>(QEvent::registerEventType());
 
 class DeeListModelPrivate {
 public:
@@ -287,12 +290,14 @@
                                            GParamSpec *pspec,
                                            DeeListModel *model)
 {
-    model->d->createRoles();
-    model->beginResetModel();
-    model->d->m_count = dee_model_get_n_rows(model->d->m_deeModel);
-    model->synchronizedChanged(model->synchronized());
-    model->endResetModel();
-    Q_EMIT model->countChanged();
+    QGlibCallbackEvent::sendEvent(model, [model](QObject*) {
+        model->d->createRoles();
+        model->beginResetModel();
+        model->d->m_count = dee_model_get_n_rows(model->d->m_deeModel);
+        model->synchronizedChanged(model->synchronized());
+        model->endResetModel();
+        Q_EMIT model->countChanged();
+    });
 }
 
 void
@@ -300,19 +305,21 @@
                                 DeeModelIter* iter,
                                 DeeListModel* model)
 {
-    if(!model->synchronized()) {
-        return;
-    }
-
-    gint position = dee_model_get_position(model->d->m_deeModel, iter);
-
-    /* if we're inside transaction, we'll consider this row hidden */
-    model->d->m_rowBeingAdded = position;
-    model->d->processChange(ChangeType::ADDITION, position);
-    if (!model->d->m_changesetInProgress) {
-        model->d->flushChanges();
-    }
-    model->d->m_rowBeingAdded = -1;
+    QGlibCallbackEvent::sendEvent(model, [model, iter](QObject*) {
+        if(!model->synchronized()) {
+            return;
+        }
+
+        gint position = dee_model_get_position(model->d->m_deeModel, iter);
+
+        /* if we're inside transaction, we'll consider this row hidden */
+        model->d->m_rowBeingAdded = position;
+        model->d->processChange(ChangeType::ADDITION, position);
+        if (!model->d->m_changesetInProgress) {
+            model->d->flushChanges();
+        }
+        model->d->m_rowBeingAdded = -1;
+    });
 }
 
 void
@@ -320,23 +327,25 @@
                                   DeeModelIter* iter,
                                   DeeListModel* model)
 {
-    if(!model->synchronized()) {
-        return;
-    }
-
-    /* Note that at this point the row is still present and valid in the DeeModel.
-       Therefore the value returned by dee_model_get_n_rows() might not be
-       what one would expect.
-       See Dee's dee_sequence_model_remove() method.
-    */
-    gint position = dee_model_get_position(model->d->m_deeModel, iter);
-
-    model->d->m_rowBeingRemoved = position;
-    model->d->processChange(ChangeType::REMOVAL, position);
-    if (!model->d->m_changesetInProgress) {
-        model->d->flushChanges();
-    }
-    model->d->m_rowBeingRemoved = -1;
+    QGlibCallbackEvent::sendEvent(model, [model, iter](QObject*) {
+        if(!model->synchronized()) {
+            return;
+        }
+
+        /* Note that at this point the row is still present and valid in the DeeModel.
+           Therefore the value returned by dee_model_get_n_rows() might not be
+           what one would expect.
+           See Dee's dee_sequence_model_remove() method.
+        */
+        gint position = dee_model_get_position(model->d->m_deeModel, iter);
+
+        model->d->m_rowBeingRemoved = position;
+        model->d->processChange(ChangeType::REMOVAL, position);
+        if (!model->d->m_changesetInProgress) {
+            model->d->flushChanges();
+        }
+        model->d->m_rowBeingRemoved = -1;
+    });
 }
 
 void
@@ -344,32 +353,38 @@
                                   DeeModelIter* iter,
                                   DeeListModel* model)
 {
-    if(!model->synchronized()) {
-        return;
-    }
-
-    /* If we're inside a transaction we need to flush the currently queued
-     * changes and emit the dataChanged signal after that */
-    if (model->d->m_changesetInProgress) {
-        model->d->flushChanges();
-    }
-
-    gint position = dee_model_get_position(model->d->m_deeModel, iter);
-    QModelIndex index = model->index(position);
-    Q_EMIT model->dataChanged(index, index);
+    QGlibCallbackEvent::sendEvent(model, [model, iter](QObject*) {
+        if(!model->synchronized()) {
+            return;
+        }
+
+        /* If we're inside a transaction we need to flush the currently queued
+         * changes and emit the dataChanged signal after that */
+        if (model->d->m_changesetInProgress) {
+            model->d->flushChanges();
+        }
+
+        gint position = dee_model_get_position(model->d->m_deeModel, iter);
+        QModelIndex index = model->index(position);
+        Q_EMIT model->dataChanged(index, index);
+    });
 }
 
 void
 DeeListModelPrivate::onStartChangeset(DeeListModel* model)
 {
-    model->d->m_changesetInProgress = true;
+    QGlibCallbackEvent::sendEvent(model, [model](QObject*) {
+        model->d->m_changesetInProgress = true;
+    });
 }
 
 void
 DeeListModelPrivate::onFinishChangeset(DeeListModel* model)
 {
-    model->d->flushChanges();
-    model->d->m_changesetInProgress = false;
+    QGlibCallbackEvent::sendEvent(model, [model](QObject*) {
+        model->d->flushChanges();
+        model->d->m_changesetInProgress = false;
+    });
 }
 
 

=== added file 'glibcallbackevent.h'
--- glibcallbackevent.h	1970-01-01 00:00:00 +0000
+++ glibcallbackevent.h	2013-11-18 11:20:05 +0000
@@ -0,0 +1,69 @@
+/*
+ * Copyright (C) 2010 Canonical, Ltd.
+ *
+ * Authors:
+ *  Florian Boucault <[e-mail]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 3.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <QtCore/QEvent>
+#include <QtCore/QPointer>
+#include <QtCore/QCoreApplication>
+
+class QGlibCallbackEvent : public QEvent
+{
+public:
+    static const QEvent::Type eventType;
+    QGlibCallbackEvent(QObject* reciever, std::function<void(QObject*)> func)
+    :   QEvent(QGlibCallbackEvent::eventType),
+        m_func(func)
+    {
+        m_handler = new QEventHandlerObject(reciever);
+    }
+
+    QObject* handler() const { return m_handler; }
+
+    void exec() {
+        if (!m_handler.isNull()) {
+            m_func(m_handler->parent());
+            m_handler->deleteLater();
+        }
+    }
+
+    static void sendEvent(QObject* reciever, std::function<void(QObject*)> func) {
+        QGlibCallbackEvent gce(reciever, func);
+        QCoreApplication::sendEvent(gce.handler(), &gce);
+    }
+
+private:
+    class QEventHandlerObject : public QObject
+    {
+    public:
+        QEventHandlerObject(QObject* parent)
+        :   QObject(parent)
+        {}
+
+        bool event(QEvent* e)
+        {
+            if (e->type() == QGlibCallbackEvent::eventType) {
+                QGlibCallbackEvent *gce = static_cast<QGlibCallbackEvent*>(e);
+                gce->exec();
+                return true;
+            }
+            return QObject::event(e);
+        }
+    };
+    QPointer<QEventHandlerObject> m_handler;
+    std::function<void(QObject*)> m_func;
+};
\ No newline at end of file

=== modified file 'tests/CMakeLists.txt'
--- tests/CMakeLists.txt	2013-09-11 12:53:28 +0000
+++ tests/CMakeLists.txt	2013-11-18 11:20:05 +0000
@@ -18,21 +18,21 @@
 add_test(NAME conversiontest COMMAND "${CMAKE_CURRENT_BINARY_DIR}/conversiontest" "-o" "${CMAKE_CURRENT_BINARY_DIR}/conversiontest-xunit.xml,xunitxml" "-o" "-,txt")
 set_property(TEST conversiontest PROPERTY ENVIRONMENT "LD_LIBRARY_PATH=.")
 
-if (WITHQT5)
-	# the test is using signal connections with lambdas, only available in Qt5
-	add_executable(signaltest signaltest.cpp)
-	target_link_libraries(signaltest ${OUR_QT_TEST_LIB} ${DEE_QT_LIBNAME})
-	set_target_properties(signaltest PROPERTIES COMPILE_FLAGS -fPIC)
-	add_test(NAME signaltest COMMAND "${CMAKE_CURRENT_BINARY_DIR}/signaltest" "-o" "${CMAKE_CURRENT_BINARY_DIR}/signaltest-xunit.xml,xunitxml" "-o" "-,txt")
-	set_property(TEST signaltest PROPERTY ENVIRONMENT "LD_LIBRARY_PATH=.")
-endif ()
-
 add_executable(test-helper test-helper.cpp)
 target_link_libraries(test-helper ${OUR_QT_CORE_LIB} ${OUR_QT_DBUS_LIB} ${DEE_LDFLAGS})
 set_target_properties(test-helper PROPERTIES COMPILE_FLAGS -fPIC)
 
-add_executable(deelistmodeltest deelistmodeltest.cpp)
-target_link_libraries(deelistmodeltest ${OUR_QT_TEST_LIB} ${OUR_QT_DBUS_LIB} ${DEE_QT_LIBNAME})
-set_target_properties(deelistmodeltest PROPERTIES COMPILE_FLAGS -fPIC)
-add_test(NAME deelistmodeltest COMMAND "dbus-test-runner" "--task" "${CMAKE_CURRENT_BINARY_DIR}/test-helper" "--task" "${CMAKE_CURRENT_BINARY_DIR}/deelistmodeltest" "-p" "-xunitxml" "-p" "-o" "-p" "deelistmodeltest-xunit.xml")
-set_property(TEST deelistmodeltest PROPERTY ENVIRONMENT "LD_LIBRARY_PATH=.")
+if (WITHQT5)
+  # the test is using signal connections with lambdas, only available in Qt5
+  add_executable(signaltest signaltest.cpp)
+  target_link_libraries(signaltest ${OUR_QT_TEST_LIB} ${DEE_QT_LIBNAME})
+  set_target_properties(signaltest PROPERTIES COMPILE_FLAGS -fPIC)
+  add_test(NAME signaltest COMMAND "${CMAKE_CURRENT_BINARY_DIR}/signaltest" "-o" "${CMAKE_CURRENT_BINARY_DIR}/signaltest-xunit.xml,xunitxml" "-o" "-,txt")
+  set_property(TEST signaltest PROPERTY ENVIRONMENT "LD_LIBRARY_PATH=.")
+
+  add_executable(deelistmodeltest deelistmodeltest.cpp)
+  target_link_libraries(deelistmodeltest ${OUR_QT_TEST_LIB} ${OUR_QT_DBUS_LIB} ${DEE_QT_LIBNAME})
+  set_target_properties(deelistmodeltest PROPERTIES COMPILE_FLAGS -fPIC)
+  add_test(NAME deelistmodeltest COMMAND "dbus-test-runner" "--task" "${CMAKE_CURRENT_BINARY_DIR}/test-helper" "--task" "${CMAKE_CURRENT_BINARY_DIR}/deelistmodeltest" "-p" "-xunitxml" "-p" "-o" "-p" "deelistmodeltest-xunit.xml")
+  set_property(TEST deelistmodeltest PROPERTY ENVIRONMENT "LD_LIBRARY_PATH=.")
+endif ()
\ No newline at end of file

=== modified file 'tests/deelistmodeltest.cpp'
--- tests/deelistmodeltest.cpp	2013-09-11 12:53:28 +0000
+++ tests/deelistmodeltest.cpp	2013-11-18 11:20:05 +0000
@@ -104,7 +104,38 @@
         QCOMPARE(model_qt.synchronized(), false);
         QCOMPARE(model_qt.rowCount(), 0);
     }
-    
+
+    void deleteLaterTest()
+    {
+        DeeListModel model_qt;
+        model_qt.setName("com.deeqt.test.model2");
+        while(!model_qt.synchronized())
+            qApp->processEvents();
+
+        QObject* object_to_delete = new QObject(this);
+        bool object_deleted = false;
+        connect(object_to_delete, &QObject::destroyed, [&] (QObject* object) {
+            object_deleted = true;
+        });
+
+        QTimer timer;
+        timer.setSingleShot(true);
+        timer.start(2000);
+
+        connect(&model_qt, &QAbstractItemModel::rowsRemoved, [&object_to_delete] (const QModelIndex &parent, int start, int end) {
+            if (object_to_delete) {
+                object_to_delete->deleteLater();
+                object_to_delete = NULL;
+            }
+        });
+
+        while(timer.isActive() && !object_deleted) {
+            qApp->processEvents();
+        }
+
+        QVERIFY(object_deleted);
+    }
+
     void cleanupTestCase()
     {
         tell_service_to_exit();

=== modified file 'tests/test-helper.cpp'
--- tests/test-helper.cpp	2012-11-30 12:56:33 +0000
+++ tests/test-helper.cpp	2013-11-18 11:20:05 +0000
@@ -17,6 +17,7 @@
 #include <QCoreApplication>
 #include <QDBusConnection>
 #include <QObject>
+#include <QTimer>
 
 #include <dee.h>
 
@@ -26,15 +27,11 @@
     Q_OBJECT
 public:
     TestControl()
+    : m_model2(NULL)
     {
         g_type_init();
-        DeeModel* model = dee_shared_model_new("com.deeqt.test.model");
-        dee_model_set_schema(model, "b", NULL);
-
-        // Doc says we need to be synchronized before doing anything
-        while(!dee_shared_model_is_synchronized(DEE_SHARED_MODEL(model)))
-            qApp->processEvents();
-
+        create_model_1();
+        create_model_2();
         QDBusConnection::sessionBus().registerService("com.deeqt.test.control");
         QDBusConnection::sessionBus().registerObject("/control", this, QDBusConnection::ExportAllSlots);
     }
@@ -44,6 +41,47 @@
     {
         qApp->quit();
     }
+
+    void onTimer()
+    {
+        static bool add = true;
+        static int iter = 0;
+
+        if (add) {
+            dee_model_append(m_model2, iter++);
+        } else {
+            dee_model_remove(m_model2, dee_model_get_first_iter(m_model2));
+        }
+        add = !add;
+    }
+
+private:
+    void create_model_1()
+    {
+        DeeModel* model = dee_shared_model_new("com.deeqt.test.model");
+        dee_model_set_schema(model, "b", NULL);
+
+        // Doc says we need to be synchronized before doing anything
+        while(!dee_shared_model_is_synchronized(DEE_SHARED_MODEL(model)))
+            qApp->processEvents();
+    }
+
+    void create_model_2()
+    {
+        m_model2 = dee_shared_model_new("com.deeqt.test.model2");
+        dee_model_set_schema(m_model2, "i", NULL);
+
+        // Doc says we need to be synchronized before doing anything
+        while(!dee_shared_model_is_synchronized(DEE_SHARED_MODEL(m_model2)))
+            qApp->processEvents();
+
+        QTimer* timer(new QTimer(this));
+        connect(timer, SIGNAL(timeout()), this, SLOT(onTimer()));
+        timer->start(100);
+    }
+
+private:
+    DeeModel* m_model2;
 };
 
 int main(int argc, char *argv[])