converter: cleanup and add support for more variant types

Approved by: Lukáš Tinkl

3 files changed, 197 insertions, 86 deletions

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 95440ca..0aef573 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -13,6 +13,7 @@ pkg_check_modules(GLIB REQUIRED glib-2.0>=2.32)
 pkg_check_modules(GIO REQUIRED gio-2.0>=2.32)
 set(CMAKE_AUTOMOC ON)
 set(CMAKE_INCLUDE_CURRENT_DIR ON)
+SET(CMAKE_CXX_FLAGS "-std=c++11")
 add_definitions(-DQT_NO_KEYWORDS)
 
 find_program(DBUS_RUNNER dbus-test-runner)
@@ -26,7 +27,7 @@ if(ENABLE_COVERAGE)
         message(FATAL_ERROR "gcov command not found")
     endif()
     SET(CMAKE_C_FLAGS "-g -O0 -Wall --coverage")
-    SET(CMAKE_CXX_FLAGS "-g -O0 -Wall --coverage")
+    SET(CMAKE_CXX_FLAGS "-g -O0 -Wall -std=c++11 --coverage")
     SET(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} --coverage")
     include(${CMAKE_SOURCE_DIR}/cmake/lcov.cmake)
 endif()
diff --git a/libqmenumodel/src/converter.cpp b/libqmenumodel/src/converter.cpp
index 527ae85..459e77f 100644
--- a/libqmenumodel/src/converter.cpp
+++ b/libqmenumodel/src/converter.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright 2012 Canonical Ltd.
+ * Copyright 2012-2016 Canonical Ltd.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU Lesser General Public License as published by
@@ -15,6 +15,7 @@
  *
  * Authors:
  *      Renato Araujo Oliveira Filho <renato@canonical.com>
+ *      Marco Trevisan <marco.trevisan@canonical.com>
  */
 
 extern "C" {
@@ -58,6 +59,26 @@ QVariant Converter::toQVariant(GVariant *value)
         gsize size = 0;
         const gchar *v = g_variant_get_string(value, &size);
         result.setValue(QString::fromUtf8(v, size));
+    } else if (g_variant_type_equal(type, G_VARIANT_TYPE_STRING_ARRAY)) {
+        gsize size = 0;
+        const gchar **sa = g_variant_get_strv(value, &size);
+        QStringList list;
+        for (gsize i = 0; i < size; ++i) {
+            list << QString::fromUtf8(sa[i]);
+        }
+        result.setValue(list);
+        g_free(sa);
+    } else if (g_variant_type_equal(type, G_VARIANT_TYPE_BYTESTRING)) {
+        result.setValue(QByteArray(g_variant_get_bytestring(value)));
+    } else if (g_variant_type_equal(type, G_VARIANT_TYPE_BYTESTRING_ARRAY)) {
+        gsize size = 0;
+        const gchar **bsa = g_variant_get_bytestring_array(value, &size);
+        QByteArrayList list;
+        for (gsize i = 0; i < size; ++i) {
+            list << bsa[i];
+        }
+        result.setValue(list);
+        g_free(bsa);
     } else if (g_variant_type_equal(type, G_VARIANT_TYPE_VARIANT)) {
         GVariant *var = g_variant_get_variant(value);
         result = toQVariant(var);
@@ -110,34 +131,12 @@ QVariant Converter::toQVariant(GVariant *value)
      * G_VARIANT_TYPE_UNIT
      * G_VARIANT_TYPE_DICT_ENTRY
      * G_VARIANT_TYPE_DICTIONARY
-     * G_VARIANT_TYPE_STRING_ARRAY
-     * G_VARIANT_TYPE_BYTESTRING
      * G_VARIANT_TYPE_OBJECT_PATH_ARRAY
-     * G_VARIANT_TYPE_BYTESTRING_ARRAY
      */
 
     return result;
 }
 
-static GVariant* toGVariant(const QString &typeName, const QVariant &value)
-{
-    if (typeName == "uchar") {
-        return g_variant_new_byte(value.value<uchar>());
-    } else if (typeName == "short") {
-        return g_variant_new_int16(value.value<short>());
-    } else if (typeName == "ushort") {
-        return g_variant_new_uint16(value.value<ushort>());
-    } else if (typeName == "long") {
-        return g_variant_new_int64(value.value<long>());
-    } else if (typeName == "ulong") {
-        return g_variant_new_uint64(value.value<ulong>());
-    } else {
-        qWarning() << "QVariant type not supported:" << typeName;
-    }
-
-    return NULL;
-}
-
 QVariant Converter::toQVariantFromVariantString(const QString &variantString)
 {
     GVariant *gvariant;
@@ -147,7 +146,7 @@ QVariant Converter::toQVariantFromVariantString(const QString &variantString)
         return QVariant();
     }
 
-    gvariant = g_variant_parse (NULL, variantString.toUtf8().data(), NULL, NULL, &error);
+    gvariant = g_variant_parse (NULL, qUtf8Printable(variantString), NULL, NULL, &error);
 
     if (error) {
         qWarning() << "Impossible to parse" << variantString << "as variant string:"<< error->message;
@@ -184,7 +183,7 @@ GVariant* Converter::toGVariant(const QVariant &value)
         result = g_variant_new_int64(value.toLongLong());
         break;
     case QVariant::String:
-        result = g_variant_new_string(value.toString().toUtf8().data());
+        result = g_variant_new_string(qUtf8Printable(value.toString()));
         break;
     case QVariant::UInt:
         result = g_variant_new_uint32(value.toUInt());
@@ -192,6 +191,21 @@ GVariant* Converter::toGVariant(const QVariant &value)
     case QVariant::ULongLong:
         result = g_variant_new_uint64(value.toULongLong());
         break;
+    case QMetaType::UChar:
+        result = g_variant_new_byte(value.value<uchar>());
+        break;
+    case QMetaType::Short:
+        result = g_variant_new_int16(value.value<short>());
+        break;
+    case QMetaType::UShort:
+        result = g_variant_new_uint16(value.value<ushort>());
+        break;
+    case QMetaType::Long:
+        result = g_variant_new_int64(value.value<long>());
+        break;
+    case QMetaType::ULong:
+        result = g_variant_new_uint64(value.value<ulong>());
+        break;
     case QVariant::Map:
     {
         GVariantBuilder *b;
@@ -201,7 +215,19 @@ GVariant* Converter::toGVariant(const QVariant &value)
         QMapIterator<QString, QVariant> i(value.toMap());
         while (i.hasNext()) {
             i.next();
-            g_variant_builder_add(b, "{sv}", i.key().toUtf8().data(), toGVariant(i.value()));
+            g_variant_builder_add(b, "{sv}", qUtf8Printable(i.key()), toGVariant(i.value()));
+        }
+        result = g_variant_builder_end(b);
+        g_variant_builder_unref(b);
+        break;
+    }
+    case QMetaType::QByteArrayList:
+    {
+        const QByteArrayList &list = qvariant_cast<QByteArrayList>(value);
+        GVariantBuilder *b = g_variant_builder_new(G_VARIANT_TYPE_BYTESTRING_ARRAY);
+
+        for (const QByteArray &ba : list) {
+            g_variant_builder_add_value(b, g_variant_new_bytestring(ba));
         }
         result = g_variant_builder_end(b);
         g_variant_builder_unref(b);
@@ -209,17 +235,28 @@ GVariant* Converter::toGVariant(const QVariant &value)
     }
     case QVariant::List:
     {
-        QVariantList lst = value.toList();
-        GVariant **vars = g_new(GVariant*, lst.size());
-        for (int i=0; i < lst.size(); i++) {
-            vars[i] = toGVariant(lst[i]);
+        GVariantBuilder *b = g_variant_builder_new(G_VARIANT_TYPE_TUPLE);
+
+        for (const QVariant &v : value.toList()) {
+            g_variant_builder_add_value(b, toGVariant(v));
         }
-        result = g_variant_new_tuple(vars, lst.size());
-        g_free(vars);
+        result = g_variant_builder_end(b);
+        g_variant_builder_unref(b);
+        break;
+    }
+    case QVariant::StringList:
+    {
+        GVariantBuilder *b = g_variant_builder_new(G_VARIANT_TYPE_STRING_ARRAY);
+
+        for (const QString &s : value.toStringList()) {
+            g_variant_builder_add(b, "s", qUtf8Printable(s));
+        }
+        result = g_variant_builder_end(b);
+        g_variant_builder_unref(b);
         break;
     }
     default:
-        result = ::toGVariant(value.typeName(), value);
+        qWarning() << "QVariant type not supported:" << value.type();
     }
 
     return result;
@@ -232,8 +269,7 @@ GVariant* Converter::toGVariantWithSchema(const QVariant &value, const char* sch
     }
 
     GVariant* result = NULL;
-    const GVariantType* schema_type;
-    schema_type = g_variant_type_new(schema);
+    GVariantType* schema_type = g_variant_type_new(schema);
 
     if (g_variant_type_equal(schema_type, G_VARIANT_TYPE_BOOLEAN)) {
         if (value.canConvert<bool>()) {
@@ -273,80 +309,65 @@ GVariant* Converter::toGVariantWithSchema(const QVariant &value, const char* sch
         }
     } else if (g_variant_type_equal(schema_type, G_VARIANT_TYPE_STRING)) {
         if (value.canConvert<QString>()) {
-            result = g_variant_new_string(value.toString().toUtf8().data());
+            result = g_variant_new_string(qUtf8Printable(value.toString()));
         }
     } else if (g_variant_type_equal(schema_type, G_VARIANT_TYPE_VARIANT)) {
-        result = Converter::toGVariant(value);
+        result = g_variant_new_variant(Converter::toGVariant(value));
     } else if (g_variant_type_equal(schema_type, G_VARIANT_TYPE_VARDICT)) {
         if (value.canConvert(QVariant::Map)) {
             result = Converter::toGVariant(value.toMap());
         }
     } else if (g_variant_type_is_array(schema_type)) {
         if (value.canConvert(QVariant::List)) {
+            const GVariantType* entryType = g_variant_type_element(schema_type);
+            const gchar* entryTypeString = g_variant_type_peek_string(entryType);
 
-            const GVariantType* entry_type;
-            GVariant* data;
-            entry_type = g_variant_type_element(schema_type);
-            gchar* entryTypeString = g_variant_type_dup_string(entry_type);
-
-            QVariantList lst = value.toList();
-            GVariant **vars = g_new(GVariant*, lst.size());
-
+            GVariantBuilder *b = g_variant_builder_new(G_VARIANT_TYPE_ARRAY);
             bool ok = true;
-            for (int i=0; i < lst.size(); i++) {
-                data = Converter::toGVariantWithSchema(lst[i], entryTypeString);
+
+            for (const QVariant &v : value.toList()) {
+                GVariant *data = toGVariantWithSchema(v, entryTypeString);
 
                 if (data) {
-                    vars[i] = data;
-                }
-                else {
+                    g_variant_builder_add_value(b, data);
+                } else {
                     ok = false;
                     qWarning() << "Failed to convert list to array with schema:" << schema;
                     break;
                 }
             }
             if (ok) {
-                result = g_variant_new_array(entry_type, vars, lst.size());
+                result = g_variant_builder_end(b);
             }
-            g_free(entryTypeString);
-            g_free(vars);
+            g_variant_builder_unref(b);
         }
     } else if (g_variant_type_is_tuple(schema_type)) {
         if (value.canConvert(QVariant::List)) {
-            GVariant* data;
-
-            QVariantList lst = value.toList();
-            GVariant **vars = g_new(GVariant*, lst.size());
-
-            const GVariantType* entry_type = g_variant_type_first(schema_type);
+            const GVariantType* entryType = g_variant_type_first(schema_type);
 
+            GVariantBuilder *b = g_variant_builder_new(G_VARIANT_TYPE_TUPLE);
             bool ok = true;
-            for (int i=0; i < lst.size(); i++) {
-
-                gchar* entryTypeString = g_variant_type_dup_string(entry_type);
 
-                data = Converter::toGVariantWithSchema(lst[i], entryTypeString);
+            for (const QVariant &v : value.toList()) {
+                gchar* entryTypeString = g_variant_type_dup_string(entryType);
+                GVariant *data = toGVariantWithSchema(v, entryTypeString);
+                g_free(entryTypeString);
 
                 if (data) {
-                    vars[i] = data;
-                }
-                else {
+                    g_variant_builder_add_value(b, data);
+                    entryType = g_variant_type_next(entryType);
+                    if (!entryType)
+                        break;
+                } else {
                     ok = false;
-                    qWarning() << "Failed to convert list to tuple with schema:" << schema;
-                    g_free(entryTypeString);
-                    break;
-                }
-                g_free(entryTypeString);
-
-                entry_type = g_variant_type_next(entry_type);
-                if (!entry_type) {
+                    qWarning() << "Failed to convert list to array with schema:" << schema;
                     break;
                 }
             }
             if (ok) {
-                result = g_variant_new_tuple(vars, lst.size());
+                result = g_variant_builder_end(b);
             }
-            g_free(vars);
+            g_variant_builder_unref(b);
         }
     }
 
@@ -354,6 +375,9 @@ GVariant* Converter::toGVariantWithSchema(const QVariant &value, const char* sch
     if (!result) {
         result = Converter::toGVariant(value);
     }
+
+    g_free(schema_type);
+
     return result;
 }
 
diff --git a/tests/client/convertertest.cpp b/tests/client/convertertest.cpp
index cd791c7..43127a7 100644
--- a/tests/client/convertertest.cpp
+++ b/tests/client/convertertest.cpp
@@ -28,13 +28,11 @@ extern "C" {
 #include <QtTest>
 #include <QDebug>
 
-class QGVariantType : public QObject
+class QGVariantType
 {
-    Q_OBJECT
 public:
     QGVariantType() : type(NULL) {}
     QGVariantType(const GVariantType *gvtype) : type(gvtype) {}
-    QGVariantType(const QGVariantType &other) : type(other.type) {}
     const GVariantType *getType() const { return type; }
     operator const GVariantType*() const { return type; }
 
@@ -43,14 +41,13 @@ private:
 };
 Q_DECLARE_METATYPE(QGVariantType);
 
-class QGVariant : public QObject
+class QGVariant
 {
-    Q_OBJECT
 public:
     QGVariant() : variant(NULL) {}
     ~QGVariant() { if (variant) g_variant_unref(variant); }
     QGVariant(GVariant *gv) : variant(g_variant_ref_sink(gv)) {}
-    QGVariant(const QGVariant &other) : variant(g_variant_ref_sink(other.variant)) {}
+    QGVariant(const QGVariant &other) : QGVariant(other.variant) {}
     GVariant *gvariant() const { return variant; }
     operator GVariant*() const { return variant; }
 
@@ -104,6 +101,7 @@ private:
                        << "Expected:"<< g_variant_type_peek_string(expected_type);
         }
         g_variant_unref(gv);
+        g_variant_type_free(expected_type);
         return result;
     }
 
@@ -130,9 +128,16 @@ private Q_SLOTS:
         QTest::newRow("UInt64") << QVariant::fromValue<qulonglong>(42) << QGVariantType(G_VARIANT_TYPE_UINT64);
         QTest::newRow("Double") << QVariant((double)42.42) << QGVariantType(G_VARIANT_TYPE_DOUBLE);
         QTest::newRow("String") << QVariant(QString("42")) << QGVariantType(G_VARIANT_TYPE_STRING);
+        QTest::newRow("String List") << QVariant(QStringList({"42", "42"})) << QGVariantType(G_VARIANT_TYPE_STRING_ARRAY);
         QTest::newRow("ByteArray") << QVariant(QByteArray("42")) << QGVariantType(G_VARIANT_TYPE_BYTESTRING);
         QTest::newRow("Map") << QVariant(QVariantMap()) << QGVariantType(G_VARIANT_TYPE_VARDICT);
+        QTest::newRow("Map Filled") << QVariant(QVariantMap({{"fooBar", 0xdeadbeef}})) << QGVariantType(G_VARIANT_TYPE_VARDICT);
         QTest::newRow("List") << QVariant(QVariantList()) << QGVariantType(G_VARIANT_TYPE_UNIT);
+        QTest::newRow("List Filled") << QVariant(QVariantList({"fooBar", 0xdeadbeef})) << QGVariantType(G_VARIANT_TYPE("(su)"));
+
+        QVariant result;
+        result.setValue(QByteArrayList({"42", "53"}));
+        QTest::newRow("ByteArrayList") << result << QGVariantType(G_VARIANT_TYPE_BYTESTRING_ARRAY);
     }
 
     void testConvertToGVariant()
@@ -143,6 +148,23 @@ private Q_SLOTS:
         QVERIFY(compare(value, expectedType));
     }
 
+    void testConvertToGVariantAndBack_data()
+    {
+        testConvertToGVariant_data();
+    }
+
+    void testConvertToGVariantAndBack()
+    {
+        QFETCH(QVariant, value);
+
+        GVariant *gv = Converter::toGVariant(value);
+        QVERIFY(gv != NULL);
+
+        QCOMPARE(Converter::toQVariant(gv), value);
+
+        g_variant_unref(gv);
+    }
+
     void testTupleConversion()
     {
         QVariantList qTuple;
@@ -175,9 +197,22 @@ private Q_SLOTS:
         QTest::addColumn<QVariant>("value");
         QTest::addColumn<QString>("schema");
 
+        // convert to byte
+        QTest::newRow("byte") << QVariant::fromValue<int>(1) << "y";
+
         // convert to integer
         QTest::newRow("integer") << QVariant::fromValue<int>(1) << "i";
         QTest::newRow("integer from double") << QVariant::fromValue<double>(1.1) << "i";
+        QTest::newRow("int16") << QVariant::fromValue<int>(-1) << "n";
+        QTest::newRow("uint16") << QVariant::fromValue<int>(1) << "q";
+        QTest::newRow("uint32") << QVariant::fromValue<int>(1) << "u";
+        QTest::newRow("int64") << QVariant::fromValue<int>(1) << "x";
+        QTest::newRow("uint64") << QVariant::fromValue<int>(1) << "t";
+
+        // convert to variant
+        QTest::newRow("variant from int") << QVariant::fromValue<int>(1) << "v";
+        QTest::newRow("variant from double") << QVariant::fromValue<double>(1.1) << "v";
+        QTest::newRow("variant from string") << QVariant::fromValue<QString>("string") << "v";
 
         // convert to bool
         QTest::newRow("bool") << QVariant::fromValue<bool>(true) << "b";
@@ -242,17 +277,24 @@ private Q_SLOTS:
         QTest::newRow("UInt64") << QGVariant(g_variant_new_uint64(53)) << (unsigned) QVariant::ULongLong;
         QTest::newRow("Double") << QGVariant(g_variant_new_double(53.3)) << (unsigned) QVariant::Double;
         QTest::newRow("String") << QGVariant(g_variant_new_string("53")) << (unsigned) QVariant::String;
+        QTest::newRow("Byte string") << QGVariant(g_variant_new_bytestring("53")) << (unsigned) QVariant::ByteArray;
         QTest::newRow("Tuple") << QGVariant(g_variant_new("(si)", "foo", 53)) << (unsigned) QVariant::List;
 
-        GVariantBuilder* builder = g_variant_builder_new(G_VARIANT_TYPE("a{sv}"));
-        g_variant_builder_add(builder, "{sv}", "fooo", g_variant_new_variant(g_variant_new_int64(53)));
-        QTest::newRow("Map") << QGVariant(g_variant_new("a{sv}", builder)) << (unsigned) QVariant::Map;
+        GVariantBuilder* builder = g_variant_builder_new(G_VARIANT_TYPE_VARDICT);
+        g_variant_builder_add(builder, "{sv}", "fooo", g_variant_new_int64(53));
+        QTest::newRow("Map") << QGVariant(g_variant_builder_end(builder)) << (unsigned) QVariant::Map;
         g_variant_builder_unref(builder);
 
         builder = g_variant_builder_new(G_VARIANT_TYPE("ai"));
         g_variant_builder_add(builder, "i", g_variant_new_int32(53));
         QTest::newRow("List") << QGVariant(g_variant_new("ai", builder)) << (unsigned) QVariant::List;
         g_variant_builder_unref(builder);
+
+        QTest::newRow("Tuple") << QGVariant(g_variant_new("(i)", 53)) << (unsigned) QVariant::List;
+
+        const gchar *byteArray[] = {"42", "53", NULL};
+        QTest::newRow("ByteArrayList") << QGVariant(g_variant_new_bytestring_array(byteArray, -1)) << (unsigned) QMetaType::QByteArrayList;
+        QTest::newRow("String List") << QGVariant(g_variant_new_strv(byteArray, -1)) << (unsigned) QVariant::StringList;
     }
 
     void testConvertToQVariant()
@@ -263,6 +305,47 @@ private Q_SLOTS:
         QVERIFY(compare(value, (QVariant::Type) expectedType));
     }
 
+    void testConvertToQVariantAndBack_data()
+    {
+        testConvertToQVariant_data();
+    }
+
+    void testConvertToQVariantAndBack()
+    {
+        QFETCH(QGVariant, value);
+
+        QVariant qv = Converter::toQVariant(value);
+        QVERIFY(qv.isValid());
+
+        GVariant *gv = Converter::toGVariant(qv);
+        gboolean equals = g_variant_equal(value, gv);
+
+        if (!equals && qv.type() == QVariant::List) {
+            QVERIFY(g_variant_type_is_array(g_variant_get_type(value)));
+            QVERIFY(g_variant_type_is_tuple(g_variant_get_type(gv)));
+
+            gsize vsize = g_variant_n_children(value);
+            QCOMPARE(vsize, g_variant_n_children(gv));
+
+            for (gsize i = 0; i < vsize; ++i) {
+                equals = g_variant_equal(g_variant_get_child_value(value, i), g_variant_get_child_value(gv, i));
+                if (!equals)
+                    break;
+            }
+        }
+
+        if (!equals) {
+            gchar *vs = g_variant_print(value, TRUE);
+            gchar *gvs = g_variant_print(gv, TRUE);
+            qWarning() << "Values do not match. Old" << vs << "converted" << gvs;
+            g_free(vs);
+            g_free(gvs);
+        }
+
+        g_variant_unref(gv);
+        QVERIFY(equals != FALSE);
+    }
+
     void testConvertToQVariantFromString_data()
     {
         QTest::addColumn<QString>("value");
@@ -279,9 +362,12 @@ private Q_SLOTS:
         QTest::newRow("Double") << "double 65" << (unsigned) QVariant::Double;
         QTest::newRow("String") << "string '65'" << (unsigned) QVariant::String;
         QTest::newRow("String simple") << "\"65\"" << (unsigned) QVariant::String;
+        QTest::newRow("String List") << "['foo', 'bar']" << (unsigned) QVariant::StringList;
+        QTest::newRow("Byte string") << "b'fooo'" << (unsigned) QVariant::ByteArray;
         QTest::newRow("Map") << "{'foo': <65>}" << (unsigned) QVariant::Map;
         QTest::newRow("List") << "[65, 66]" << (unsigned) QVariant::List;
         QTest::newRow("Tuple") << "('foo', 65)" << (unsigned) QVariant::List;
+        QTest::newRow("ByteArrayList") << "[b'foo', b'bar']" << (unsigned) QMetaType::QByteArrayList;
     }
 
     void testConvertToQVariantFromString()