Fix review comments

Pelagicore · Sep 30, 2020 · 164a6b9 · 164a6b9
1 parent a3e6151
commit 164a6b9
Show file tree

Hide file tree

Showing 49 changed files with 931 additions and 703 deletions.
diff --git a/codegen/facelift/facelift-codegen.py b/codegen/facelift/facelift-codegen.py
@@ -132,7 +132,7 @@ def requiredIncludeFromType(symbol, suffix):
 
 def insertUniqueType(symbol, unique_types):
     type = symbol.type.nested if symbol.type.nested else symbol.type
-    if type not in (t.name for t in unique_types):
+    if type.name not in (t.name for t in unique_types):
         unique_types.append(type)
 
 def referencedTypes(self):
@@ -149,21 +149,23 @@ def referencedTypes(self):
             insertUniqueType(param, types)
     return types
 
-def appendTypeIfStructure(symbol, list):
+def appendTypeIfStructureAndUnique(symbol, unique_list):
     type = symbol.type.nested if symbol.type.nested else symbol.type
-    if type.is_struct:
-        list.append(type)
+    if type.is_struct and type.name not in (t.name for t in unique_list):
+        unique_list.append(type)
 
 def referencedStructureTypes(self):
     interfaces = []
     for property in self.properties:
-        appendTypeIfStructure(property, interfaces)
+        appendTypeIfStructureAndUnique(property, interfaces)
     for m in self.operations:
         for param in m.parameters:
-            appendTypeIfStructure(param, interfaces)
+            appendTypeIfStructureAndUnique(param, interfaces)
+        if m.hasReturnValue:
+            appendTypeIfStructureAndUnique(m.type, interfaces)
     for m in self.signals:
         for param in m.parameters:
-            appendTypeIfStructure(param, interfaces)
+            appendTypeIfStructureAndUnique(param, interfaces)
     return interfaces
 
 def appendTypeIfInterface(symbol, list):
@@ -211,8 +213,8 @@ def isQMLImplementationEnabled(self):
 def isSerializable(self):
     return True if self.tags.get('serializable') else generateAll
 
-def serializeOverIPC(self):
-    return True if self.tags.get('serializeOverIPC') else generateAll
+def toByteArrayOverDBus(self):
+    return True if self.tags.get('toByteArrayOverDBus') else generateAll
 
 def isQObjectWrapperEnabled(self):
     return True if self.tags.get('qml-component') else False
@@ -312,7 +314,7 @@ def cppMethodArgumentType(self):
 
 setattr(qface.idl.domain.Struct, 'verifyStruct', property(verifyStruct))
 setattr(qface.idl.domain.Struct, 'isSerializable', property(isSerializable))
-setattr(qface.idl.domain.Struct, 'serializeOverIPC', property(serializeOverIPC))
+setattr(qface.idl.domain.Struct, 'toByteArrayOverDBus', property(toByteArrayOverDBus))
 setattr(qface.idl.domain.Struct, 'isQObjectWrapperEnabled', property(isQObjectWrapperEnabled))
 setattr(qface.idl.domain.Struct, 'isQObjectWrapperDeprecated', property(isQObjectWrapperDeprecated))
 
@@ -371,6 +373,7 @@ def run_generation(input, output, dependency, libraryName, all):
             generateFile(generator, 'module/{{path}}/Module.cpp', 'Module.template.cpp', ctx, libraryName, "")
             generateFile(generator, 'ipc/{{path}}/ModuleIPC.h', 'ModuleIPC.template.h', ctx, libraryName, "")
             generateFile(generator, 'ipc/{{path}}/ModuleIPC.cpp', 'ModuleIPC.template.cpp', ctx, libraryName, "")
+
             for interface in module.interfaces:
                 log.debug('process interface %s' % interface)
                 ctx.update({'interface': interface})

diff --git a/codegen/facelift/templates/IPCAdapter.template.cpp b/codegen/facelift/templates/IPCAdapter.template.cpp
@@ -39,31 +39,10 @@
 #include "InterfaceManager.h"
 
 #ifdef DBUS_IPC_ENABLED
-#include <QtDBus>
 #include "{{module.fullyQualifiedPath}}/{{interfaceName}}IPCDBusAdapter.h"
-{% for struct in module.structs %}
-#include "{{struct.fullyQualifiedPath}}.h"
-{% endfor %}
-
-{% for enum in module.enums %}
-#include "{{enum.fullyQualifiedPath}}.h"
-{% endfor %}
-
 {% for property in interface.referencedInterfaceTypes %}
 #include "{{property.fullyQualifiedPath}}{% if generateAsyncProxy %}Async{% endif %}IPCDBusAdapter.h"
 {% endfor %}
-
-#define DBUS_MAXIMUM_SIGNATURE_LENGTH 255
-
-{% for type in interface.referencedTypes %}
-{% if (not type.is_primitive) %}
-{% if (not type.is_model) %}
-{% if (not type.is_interface) %}
-{{type.requiredInclude}}
-{% endif %}
-{% endif %}
-{% endif %}
-{% endfor %}
 #endif
 
 {{module.namespaceCppOpen}}
@@ -101,31 +80,6 @@ struct {{interfaceName}}IPCAdapter::Impl {
 {{interfaceName}}IPCAdapter::{{interfaceName}}IPCAdapter(QObject* parent) :
     BaseType(facelift::InterfaceManager::instance(), parent)
 {
-#ifdef DBUS_IPC_ENABLED
-    {% for type in interface.referencedTypes %}
-    {% if (not type.is_primitive) %}
-    {% if (not type.is_enum) %}
-    {% if (not type.is_model) %}
-    {% if (not type.is_interface) %}
-    qDBusRegisterMetaType<{{type.fullyQualifiedCppType}}>();
-    qDBusRegisterMetaType<QMap<QString,{{type.fullyQualifiedCppType}}>>();
-    qDBusRegisterMetaType<QList<{{type.fullyQualifiedCppType}}>>();
-    {% if type.is_struct %}
-    {{type.fullyQualifiedCppType}}::registerDBusTypes();
-    Q_ASSERT_X(strlen(QDBusMetaType::typeToSignature(qMetaTypeId<{{type.fullyQualifiedCppType}}>())) < DBUS_MAXIMUM_SIGNATURE_LENGTH, "Signature overflow",
-             "Struct's signature exceeds dbus limit, annonate @serializeOverIPC to switch to byte array stream of the struct over dbus, but yet better rethink your structure!");
-    {% endif %}
-    {% if type.nested.is_struct %}
-    {{type.nested.fullyQualifiedCppType}}::registerDBusTypes();
-    Q_ASSERT_X(strlen(QDBusMetaType::typeToSignature(qMetaTypeId<{{type.fullyQualifiedCppType}}>())) < DBUS_MAXIMUM_SIGNATURE_LENGTH, "Signature overflow",
-             "Struct's signature exceeds dbus limit, annonate @serializeOverIPC to switch to binary array stream of the struct over dbus, but yet better rethink your structure!");
-    {% endif %}
-    {% endif %}
-    {% endif %}
-    {% endif %}
-    {% endif %}
-    {% endfor %}
-#endif
 }
 
 {{interfaceName}}IPCAdapter::~{{interfaceName}}IPCAdapter() {

diff --git a/codegen/facelift/templates/IPCDBusServiceAdapter.template.h b/codegen/facelift/templates/IPCDBusServiceAdapter.template.h
@@ -44,6 +44,7 @@
 #include "IPCDBusServiceAdapter.h"
 #include "IPCAdapterModelPropertyHandler.h"
 #include "DBusManager.h"
+#include "FaceliftDBusMarshaller.h"
 
 #include "{{module.fullyQualifiedPath}}/{{interfaceName}}.h"
 #include "{{module.fullyQualifiedPath}}/{{interfaceName}}QMLAdapter.h"
@@ -86,13 +87,13 @@ class {{classExport}} {{className}}: public {{baseClass}}
 
     void connectSignals() override;
 
-    QMap<QString, QDBusVariant> changedProperties();
+    QVariantMap changedProperties();
 
-    void marshalPropertyValues(const QList<QVariant>& arguments, OutputIPCMessage& msg) override;
+    QVariantMap marshalProperties() override;
 
-    void marshalProperty(const QList<QVariant>& arguments, OutputIPCMessage& msg) override;
+    QVariant marshalProperty(const QString& propertyName) override;
 
-    void setProperty(const QList<QVariant>& arguments) override;
+    void setProperty(const QString& propertyName, const QVariant& value) override;
 
     {% for event in interface.signals %}
     void {{event}}(

diff --git a/codegen/facelift/templates/IPCProxy.template.cpp b/codegen/facelift/templates/IPCProxy.template.cpp
@@ -39,15 +39,6 @@
 
 #ifdef DBUS_IPC_ENABLED
 #include "{{module.fullyQualifiedPath}}/{{interfaceName}}IPCDBusProxy.h"
-{% for type in interface.referencedTypes %}
-{% if (not type.is_primitive) %}
-{% if (not type.is_model) %}
-{% if (not type.is_interface) %}
-{{type.requiredInclude}}
-{% endif %}
-{% endif %}
-{% endif %}
-{% endfor %}
 #endif
 
 {% set className = interfaceName + "IPCProxy" %}
@@ -90,22 +81,6 @@ struct {{className}}::Impl {
 {{className}}::{{className}}(QObject *parent) : BaseType(facelift::InterfaceManager::instance(), parent),
     m_impl(std::make_unique<Impl>())
 {
-#ifdef DBUS_IPC_ENABLED
-    {% for type in interface.referencedTypes %}
-    {% if (not type.is_primitive) %}
-    {% if (not type.is_enum) %}
-    {% if (not type.is_model) %}
-    {% if (not type.is_interface) %}
-    qDBusRegisterMetaType<{{type.fullyQualifiedCppType}}>();
-    qDBusRegisterMetaType<QMap<QString,{{type.fullyQualifiedCppType}}>>();
-    qDBusRegisterMetaType<QList<{{type.fullyQualifiedCppType}}>>();
-    {% endif %}
-    {% endif %}
-    {% endif %}
-    {% endif %}
-    {% endfor %}
-#endif
-
     ipc()->setObjectPath(SINGLETON_OBJECT_PATH);
 
     {% if generateAsyncProxy %}

diff --git a/codegen/facelift/templates/IPCProxyAdapter.template.cpp b/codegen/facelift/templates/IPCProxyAdapter.template.cpp
@@ -36,8 +36,6 @@
 {% set className = interfaceName + proxyTypeNameSuffix %}
 
 #include "{{className}}.h"
-#include "FaceliftEnum.h"
-
 {{module.namespaceCppOpen}}
 
 {{className}}::{{className}}(QObject *parent) : BaseType(parent)
@@ -53,53 +51,56 @@
     {% endif %}
 }
 
-void {{className}}::unmarshalPropertyValues(InputIPCMessage &msg)
+void {{className}}::unmarshalProperties(const QVariantMap& values)
 {
-    QListIterator<QVariant> argumentsIterator(msg.arguments());
-    if (argumentsIterator.hasNext()) {
-        QMap<QString, QVariant> values = castFromVariant<QMap<QString, QVariant>>(argumentsIterator.next());
-        for (const QString &propertyName: values.keys()) {
-            {% for property in interface.properties %}
-            if (propertyName == QStringLiteral("{{property.name}}")) {
-            {% if property.type.is_interface %}
-                const {{property.interfaceCppType}} previous_{{property.name}}_Value = m_{{property.name}};
-                m_{{property.name}} = castFromDBusVariant<{{property.interfaceCppType}}>(values[propertyName]);
-                bool emit_{{property.name}}ChangeSignal = ((previous_{{property.name}}_Value != m_{{property.name}}));
-            {% elif property.type.is_model %}
-                bool emit_{{property.name}}ChangeSignal = true;
-                int {{property.name}}Size = castFromDBusVariant<int>(values[propertyName]);
-                m_{{property.name}}.beginResetModel();
-                m_{{property.name}}.reset({{property.name}}Size, std::bind(&ThisType::{{property.name}}Data, this, std::placeholders::_1));
-                m_{{property.name}}.endResetModel();
-            {% else %}
-                const auto previous_{{property.name}}_Value = m_{{property.name}};
-                m_{{property.name}} = castFromDBusVariant<{{property.interfaceCppType}}>(values[propertyName]);
-                bool emit_{{property.name}}ChangeSignal = ((previous_{{property.name}}_Value != m_{{property.name}}));
-            {% endif %}
-                if (emit_{{property.name}}ChangeSignal)
-                    emit {{property.name}}Changed();
-            }
-            {% endfor %}
-            if (propertyName == QStringLiteral("ready")) {
-                bool previousIsReady = this->ready();
-                m_serviceReady = castFromDBusVariant<bool>(values[propertyName]);
-                bool emit_ReadyChangeSignal = (previousIsReady != m_serviceReady);
-                if (emit_ReadyChangeSignal)
-                    emit readyChanged();
-            }
+    QMap<QString, bool> emitChangeSignal;
+    for (const QString &propertyName: values.keys()) {
+        {% for property in interface.properties %}
+        if (propertyName == QStringLiteral("{{property.name}}")) {
+        {% if property.type.is_interface %}
+            const {{property.interfaceCppType}} previous_{{property.name}}_Value = m_{{property.name}};
+            m_{{property.name}} = castFromQVariant<{{property.interfaceCppType}}>(values[propertyName]);
+            emitChangeSignal[QStringLiteral("{{property.name}}")] = ((previous_{{property.name}}_Value != m_{{property.name}}));
+        {% elif property.type.is_model %}
+            emitChangeSignal[QStringLiteral("{{property.name}}")] = true;
+            int {{property.name}}Size = castFromQVariant<int>(values[propertyName]);
+            m_{{property.name}}.beginResetModel();
+            m_{{property.name}}.reset({{property.name}}Size, std::bind(&ThisType::{{property.name}}Data, this, std::placeholders::_1));
+            m_{{property.name}}.endResetModel();
+        {% else %}
+            const auto previous_{{property.name}}_Value = m_{{property.name}};
+            m_{{property.name}} = castFromQVariant<{{property.interfaceCppType}}>(values[propertyName]);
+            emitChangeSignal[QStringLiteral("{{property.name}}")] = ((previous_{{property.name}}_Value != m_{{property.name}}));
+        {% endif %}
+        }
+        {% endfor %}
+        if (propertyName == QStringLiteral("ready")) {
+            bool previousIsReady = this->ready();
+            m_serviceReady = castFromQVariant<bool>(values[propertyName]);
+            emitChangeSignal[QStringLiteral("ready")] = (previousIsReady != m_serviceReady);
+        }
+    }
+    for (const QString &propertyName: emitChangeSignal.keys()) {
+        {% for property in interface.properties %}
+        if (propertyName == QStringLiteral("{{property.name}}") && emitChangeSignal[propertyName]) {
+            emit {{property.name}}Changed();
+        }
+        {% endfor %}
+        if (propertyName == QStringLiteral("ready") && emitChangeSignal[propertyName]) {
+            emit readyChanged();
         }
     }
 }
 
 void {{className}}::handleSignals(InputIPCMessage& msg)
 {
-    Q_UNUSED(msg);
+    Q_UNUSED(msg)
     {% for event in interface.signals %}
     if (msg.member() == QStringLiteral("{{event}}")) {
         QListIterator<QVariant> argumentsIterator(msg.arguments());
         {% for parameter in event.parameters %}
         {{parameter.interfaceCppType}} param_{{parameter.name}};
-        param_{{parameter.name}} = (argumentsIterator.hasNext() ? castFromVariant<{{parameter.interfaceCppType}}>(argumentsIterator.next()):{% if not parameter.type.is_interface %}{{parameter.interfaceCppType}}(){% else %}nullptr{% endif %});
+        param_{{parameter.name}} = (argumentsIterator.hasNext() ? castFromQVariant<{{parameter.interfaceCppType}}>(argumentsIterator.next()):{% if not parameter.type.is_interface %}{{parameter.interfaceCppType}}(){% else %}nullptr{% endif %});
         {% endfor %}
         emit {{event}}(
         {%- set comma = joiner(", ") -%}
@@ -121,11 +122,11 @@ const QList<QString>& {{className}}::getSignals() const
     "{{event}}",
     {% endfor %}
     {% if interface.hasModelProperty %}
-    "ModelUpdateEventDataChanged",
-    "ModelUpdateEventInsert",
-    "ModelUpdateEventRemove",
-    "ModelUpdateEventMove",
-    "ModelUpdateEventReset"
+    facelift::IPCCommon::MODEL_DATA_CHANGED_MESSAGE_NAME,
+    facelift::IPCCommon::MODEL_INSERT_MESSAGE_NAME,
+    facelift::IPCCommon::MODEL_REMOVE_MESSAGE_NAME,
+    facelift::IPCCommon::MODEL_MOVE_MESSAGE_NAME,
+    facelift::IPCCommon::MODEL_RESET_MESSAGE_NAME,
     {% endif %}
     };
 
@@ -136,7 +137,7 @@ const QList<QString>& {{className}}::getSignals() const
 void {{className}}::onModelUpdateEvent(const InputIPCMessage& msg)
 {
     QListIterator<QVariant> argumentsIterator(msg.arguments());
-    const QString& modelPropertyName = (argumentsIterator.hasNext() ? castFromVariant<QString>(argumentsIterator.next()): QString());
+    const QString& modelPropertyName = (argumentsIterator.hasNext() ? castFromQVariant<QString>(argumentsIterator.next()): QString());
     {% for property in interface.properties %}
     {% if property.type.is_model %}
     if (modelPropertyName == QStringLiteral("{{property.name}}")) {
@@ -147,22 +148,19 @@ void {{className}}::onModelUpdateEvent(const InputIPCMessage& msg)
 }
 {% endif %}
 
-void {{className}}::unmarshalPropertiesChanged(InputIPCMessage &msg)
+void {{className}}::unmarshalPropertiesChanged(const QVariantMap& changedProperties)
 {
     {% if interface.properties %}
-    QListIterator<QVariant> argumentsIterator(msg.arguments());
-    QString interfaceName = (argumentsIterator.hasNext() ? castFromVariant<QString>(argumentsIterator.next()): QString());
-    QVariantMap changedProperties = (argumentsIterator.hasNext() ? castFromVariant<QVariantMap>(argumentsIterator.next()): QVariantMap());
     for (const QString &propertyName: changedProperties.keys()) {
         {% for property in interface.properties %}
         if (propertyName == QStringLiteral("{{property.name}}")) {
         {% if property.type.is_model %}
-            int {{property.name}}Size = castFromDBusVariant<int>(changedProperties[propertyName]);
+            int {{property.name}}Size = castFromQVariant<int>(changedProperties[propertyName]);
             m_{{property.name}}.beginResetModel();
             m_{{property.name}}.reset({{property.name}}Size, std::bind(&ThisType::{{property.name}}Data, this, std::placeholders::_1));
             m_{{property.name}}.endResetModel();
         {% else %}
-            m_{{property.name}} = castFromDBusVariant<{{property.interfaceCppType}}>(changedProperties[propertyName]);
+            m_{{property.name}} = castFromQVariant<{{property.interfaceCppType}}>(changedProperties[propertyName]);
         {% endif %}
         }
         {% endfor %}
@@ -177,7 +175,7 @@ void {{className}}::unmarshalPropertiesChanged(InputIPCMessage &msg)
         {% endfor %}
     }
     {% else %}
-    Q_UNUSED(msg);
+    Q_UNUSED(changedProperties);
     {% endif %}
 }
 
@@ -226,7 +224,7 @@ void {{className}}::{{operation.name}}(
             {%- for parameter in operation.parameters -%}
             , {{parameter.name}}
             {%- endfor -%} );
-        return (!args.isEmpty() ? castFromVariant<{{operation.interfaceCppType}}>(args[0]):{% if not (operation.type.is_interface) %}{{operation.cppType}}(){% else %}nullptr{% endif %});
+        return (!args.isEmpty() ? castFromQVariant<{{operation.interfaceCppType}}>(args.first()):{% if not (operation.type.is_interface) %}{{operation.cppType}}(){% else %}nullptr{% endif %});
         {% else %}
         ipc()->sendMethodCall(memberID(MethodID::{{operation.name}}, "{{operation.name}}")
         {%- for parameter in operation.parameters -%}

diff --git a/codegen/facelift/templates/IPCProxyAdapter.template.h b/codegen/facelift/templates/IPCProxyAdapter.template.h
@@ -67,7 +67,7 @@ class {{classExport}} {{className}} : public {{baseClass}}
 
     {{className}}(QObject *parent = nullptr);
 
-    void unmarshalPropertyValues(InputIPCMessage &msg) override;
+    void unmarshalProperties(const QVariantMap& values) override;
 
     {% if interface.hasModelProperty %}
     void setServiceRegistered(bool isRegistered) override
@@ -85,9 +85,10 @@ class {{classExport}} {{className}} : public {{baseClass}}
     {% endif %}
 
     void handleSignals(InputIPCMessage& msg) override;
+
     const QList<QString>& getSignals() const override;
 
-    void unmarshalPropertiesChanged(InputIPCMessage &msg) override;
+    void unmarshalPropertiesChanged(const QVariantMap& changedProperties) override;
 
     {% for operation in interface.operations %}