From 39169acf3bc2bc43cc32a6455d43e9588765c84a Mon Sep 17 00:00:00 2001 From: Stanislaw Halik Date: Thu, 25 Aug 2016 11:58:24 +0200 Subject: options: use non-generic comparison for bundle modification check The generic QVariant comparison works badly for QList. Create a comparator between two QVariants for base_value in value ctor, using QVariant::value 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 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. --- options/bundle.cpp | 32 +++++++++++++++++++++++++++----- options/connector.cpp | 34 +++++++++++++++++++++++++++++----- options/connector.hpp | 11 ++++++++++- options/group.cpp | 24 ------------------------ options/group.hpp | 5 +---- options/value.hpp | 32 ++++++++++++++++++++++++-------- 6 files changed, 91 insertions(+), 47 deletions(-) (limited to 'options') 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(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(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 + 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& values = (*it).second; + std::vector& 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& values = (*it).second; + tt& tmp = (*it).second; + std::type_index& typeidx = std::get<2>(tmp); + std::vector& 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 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 #include +#include +#include +#include +#include #include #include #include @@ -18,7 +22,10 @@ class OPENTRACK_OPTIONS_EXPORT connector { friend class ::options::base_value; - std::map> connected_values; + using value_vec = std::vector; + using comparator = bool(*)(const QVariant&, const QVariant&); + using tt = std::tuple; + std::map 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 group::ini_file() return std::make_shared(); } -bool group::operator==(const group& other) const -{ - for (const auto& kv : kvs) - { - const QVariant val = other.get(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(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 kvs; QString name; public: + std::map 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 ini_file(); - bool operator==(const group& other) const; - bool operator!=(const group& other) const { return !(*this == other); } template 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 +#include +#include #include #include #include @@ -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 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::value>::ty using type = int; }; +template<> struct value_element_type { using type = double; }; + template using value_element_type_t = typename value_element_type::type; } @@ -132,16 +140,24 @@ class value final : public base_value public: using element_type = detail::value_element_type_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() == val2.value(); + } + t operator=(const t& datum) { - store(static_cast(datum)); + const element_type tmp = static_cast(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()), -- cgit v1.2.3