options: use non-generic comparison for bundle modification check

The generic QVariant comparison works badly for QList<QPointF>.

Create a comparator between two QVariants for base_value in value<tp>
ctor, using QVariant::value<tp> which returns right results once it's
converted to tp.

If a value was registered for a name in a bundle, use that comparator as
the comparator for that name. In case conflicting value types were
registered always use generic comparison for that name.

std::type_index needs to be used here since value<t> can be instantiated
in different modules (libraries), resulting in different value for the
comparator function pointer.

Move group::operator== to bundle type to avoid circular include for
connector.h.

Also use element_type more consistently in value<tp>.

6 files changed, 91 insertions, 47 deletions

diff --git a/options/bundle.cpp b/options/bundle.cpp
index aa7d0ea8..53298d5d 100644
--- a/options/bundle.cpp
+++ b/options/bundle.cpp
@@ -23,7 +23,7 @@ void bundle::reload()
     {
         QMutexLocker l(&mtx);
         saved = group(group_name);
-        const bool has_changes = transient != saved;
+        const bool has_changes = is_modified();
         transient = saved;
 
         if (has_changes)
@@ -58,11 +58,12 @@ void bundle::save_deferred(QSettings& s)
     if (group_name.size() == 0)
         return;
 
-    bool modified_ = false;
+    bool modified_ = is_modified();
 
+    if (modified_)
     {
         QMutexLocker l(&mtx);
-        if (saved != transient)
+        if (is_modified())
         {
             qDebug() << "bundle" << group_name << "changed, saving";
             modified_ = true;
@@ -80,10 +81,31 @@ void bundle::save()
     save_deferred(*group::ini_file());
 }
 
-bool bundle::is_modified() const // XXX unused
+bool bundle::is_modified() const
 {
     QMutexLocker l(mtx);
-    return transient != saved;
+
+    for (const auto& kv : transient.kvs)
+    {
+        const QVariant other = saved.get<QVariant>(kv.first);
+        if (!saved.contains(kv.first) || !is_equal(kv.first, kv.second, other))
+        {
+            qDebug() << "bundle" << group_name << "modified" << "key" << kv.first << "-" << other << "<>" << kv.second;
+            return true;
+        }
+    }
+
+    for (const auto& kv : saved.kvs)
+    {
+        const QVariant other = transient.get<QVariant>(kv.first);
+        if (!transient.contains(kv.first) || !is_equal(kv.first, kv.second, other))
+        {
+            qDebug() << "bundle" << group_name << "modified" << "key" << kv.first << "-" << other << "<>" << kv.second;
+            return true;
+        }
+    }
+
+    return false;
 }
 
 void bundler::bundle_decf(const bundler::k& key)
diff --git a/options/connector.cpp b/options/connector.cpp
index 680283cf..2f4bb0af 100644
--- a/options/connector.cpp
+++ b/options/connector.cpp
@@ -2,11 +2,30 @@
 #include "connector.hpp"
 #include "value.hpp"
 
+#include <utility>
+
 namespace options {
 namespace detail {
 
+static bool generic_is_equal(const QVariant& val1, const QVariant& val2)
+{
+    return val1 == val2;
+}
+
 connector::~connector() {}
 
+bool connector::is_equal(const QString& name, const QVariant& val1, const QVariant& val2) const
+{
+    QMutexLocker l(get_mtx());
+
+    auto it = connected_values.find(name);
+
+    if (it != connected_values.cend() && std::get<0>((*it).second).size() != 0)
+        return std::get<1>((*it).second)(val1, val2);
+    else
+        return generic_is_equal(val1, val2);
+}
+
 bool connector::on_value_destructed_impl(const QString& name, const base_value* val)
 {
     QMutexLocker l(get_mtx());
@@ -16,7 +35,7 @@ bool connector::on_value_destructed_impl(const QString& name, const base_value*
 
         if (it != connected_values.end())
         {
-            std::vector<const base_value*>& values = (*it).second;
+            std::vector<const base_value*>& values = std::get<0>((*it).second);
             for (auto it = values.begin(); it != values.end(); it++)
             {
                 if (*it == val)
@@ -69,14 +88,19 @@ void connector::on_value_created(const QString& name, const base_value* val)
 
     if (it != connected_values.end())
     {
-        std::vector<const base_value*>& values = (*it).second;
+        tt& tmp = (*it).second;
+        std::type_index& typeidx = std::get<2>(tmp);
+        std::vector<const base_value*>& values = std::get<0>(tmp);
+
+        if (typeidx != val->type_index)
+            std::get<1>((*it).second) = generic_is_equal;
         values.push_back(val);
     }
     else
     {
         std::vector<const base_value*> vec;
         vec.push_back(val);
-        connected_values[name] = vec;
+        connected_values.emplace(name, tt(vec, val->cmp, val->type_index));
     }
 }
 
@@ -85,7 +109,7 @@ void connector::notify_values(const QString& name) const
     auto it = connected_values.find(name);
     if (it != connected_values.cend())
     {
-        for (const base_value* val : (*it).second)
+        for (const base_value* val : std::get<0>((*it).second))
         {
             val->bundle_value_changed();
         }
@@ -95,7 +119,7 @@ void connector::notify_values(const QString& name) const
 void connector::notify_all_values() const
 {
     for (auto& pair : connected_values)
-        for (const base_value* val : pair.second)
+        for (const base_value* val : std::get<0>(pair.second))
             val->bundle_value_changed();
 }
 
diff --git a/options/connector.hpp b/options/connector.hpp
index b9b1259f..6aba79e2 100644
--- a/options/connector.hpp
+++ b/options/connector.hpp
@@ -2,6 +2,10 @@
 
 #include <map>
 #include <vector>
+#include <tuple>
+#include <typeinfo>
+#include <typeindex>
+#include <QVariant>
 #include <QString>
 #include <QMutex>
 #include <QMutexLocker>
@@ -18,7 +22,10 @@ class OPENTRACK_OPTIONS_EXPORT connector
 {
     friend class ::options::base_value;
 
-    std::map<QString, std::vector<const base_value*>> connected_values;
+    using value_vec = std::vector<const base_value*>;
+    using comparator = bool(*)(const QVariant&, const QVariant&);
+    using tt = std::tuple<value_vec, comparator, std::type_index>;
+    std::map<QString, tt> connected_values;
 
     void on_value_destructed(const QString& name, const base_value* val);
     void on_value_created(const QString& name, const base_value* val);
@@ -33,6 +40,8 @@ public:
     connector();
     virtual ~connector();
 
+    bool is_equal(const QString& name, const QVariant& val1, const QVariant& val2) const;
+
     connector(const connector&) = default;
     connector& operator=(const connector&) = default;
     connector(connector&&) = default;
diff --git a/options/group.cpp b/options/group.cpp
index d710afad..a2169ae5 100644
--- a/options/group.cpp
+++ b/options/group.cpp
@@ -91,28 +91,4 @@ const std::shared_ptr<QSettings> group::ini_file()
     return std::make_shared<QSettings>();
 }
 
-bool group::operator==(const group& other) const
-{
-    for (const auto& kv : kvs)
-    {
-        const QVariant val = other.get<QVariant>(kv.first);
-        if (!other.contains(kv.first) || kv.second != val)
-        {
-            qDebug() << "bundle" << name << "modified" << "key" << kv.first << "-" << val << "<>" << kv.second;
-            return false;
-        }
-    }
-
-    for (const auto& kv : other.kvs)
-    {
-        const QVariant val = get<QVariant>(kv.first);
-        if (!contains(kv.first) || kv.second != val)
-        {
-            qDebug() << "bundle" << name << "modified" << "key" << kv.first << "-" << kv.second << "<>" << val;
-            return false;
-        }
-    }
-    return true;
-}
-
 }
diff --git a/options/group.hpp b/options/group.hpp
index 05ef3b4b..f76978ad 100644
--- a/options/group.hpp
+++ b/options/group.hpp
@@ -14,10 +14,9 @@ namespace options {
 // snapshot of qsettings group at given time
 class OPENTRACK_OPTIONS_EXPORT group final
 {
-private:
-    std::map<QString, QVariant> kvs;
     QString name;
 public:
+    std::map<QString, QVariant> kvs;
     group(const QString& name);
     void save() const;
     void save_deferred(QSettings& s) const;
@@ -28,8 +27,6 @@ public:
     static QString ini_pathname();
     static const QStringList ini_list();
     static const std::shared_ptr<QSettings> ini_file();
-    bool operator==(const group& other) const;
-    bool operator!=(const group& other) const { return !(*this == other); }
 
     template<typename t>
     t get(const QString& k) const
diff --git a/options/value.hpp b/options/value.hpp
index 4ed61f1c..6942ed88 100644
--- a/options/value.hpp
+++ b/options/value.hpp
@@ -5,6 +5,8 @@
 #include "bundle.hpp"
 #include "slider.hpp"
 #include <type_traits>
+#include <typeinfo>
+#include <typeindex>
 #include <QString>
 #include <QPointF>
 #include <QList>
@@ -28,9 +30,16 @@ class OPENTRACK_OPTIONS_EXPORT base_value : public QObject
 {
     Q_OBJECT
     friend class ::options::detail::connector;
+
+    using comparator = bool(*)(const QVariant& val1, const QVariant& val2);
+
 public:
     QString name() const { return self_name; }
-    base_value(bundle b, const QString& name) : b(b), self_name(name)
+    base_value(bundle b, const QString& name, comparator cmp, std::type_index type_idx) :
+        b(b),
+        self_name(name),
+        cmp(cmp),
+        type_index(type_idx)
     {
         b->on_value_created(name, this);
     }
@@ -57,6 +66,8 @@ signals:
 protected:
     bundle b;
     QString self_name;
+    comparator cmp;
+    std::type_index type_index;
 
     template<typename t>
     void store(const t& datum)
@@ -64,11 +75,6 @@ protected:
         b->store_kv(self_name, QVariant::fromValue(datum));
     }
 
-    void store(float datum)
-    {
-        store(double(datum));
-    }
-
 public slots:
     OPENTRACK_DEFINE_SLOT(double)
     OPENTRACK_DEFINE_SLOT(int)
@@ -122,6 +128,8 @@ struct value_element_type<t, typename std::enable_if<std::is_enum<t>::value>::ty
     using type = int;
 };
 
+template<> struct value_element_type<float, void> { using type = double; };
+
 template<typename t> using value_element_type_t = typename value_element_type<t>::type;
 
 }
@@ -132,16 +140,24 @@ class value final : public base_value
 public:
     using element_type = detail::value_element_type_t<t>;
 
+    // XXX pointer comparison is wrong, need typeid since is_equal in one module doesn't equal in another!
+    static bool is_equal(const QVariant& val1, const QVariant& val2)
+    {
+        return val1.value<element_type>() == val2.value<element_type>();
+    }
+
     t operator=(const t& datum)
     {
-        store(static_cast<element_type>(datum));
+        const element_type tmp = static_cast<element_type>(datum);
+        if (tmp != get())
+            store(tmp);
         return datum;
     }
 
     static constexpr const Qt::ConnectionType DIRECT_CONNTYPE = Qt::AutoConnection;
     static constexpr const Qt::ConnectionType SAFE_CONNTYPE = Qt::QueuedConnection;
 
-    value(bundle b, const QString& name, t def) : base_value(b, name), def(def)
+    value(bundle b, const QString& name, t def) : base_value(b, name, &is_equal, std::type_index(typeid(element_type))), def(def)
     {
         QObject::connect(b.get(), SIGNAL(reloading()),
                          this, SLOT(reload()),