spline-widget: multiple fixes

- add missing locking
- bundle and settings ptr are never null following initialization;
  sentinel objects get created if there's no bundle, thus don't branch
  outside initialization functions. the sentinel object doesn't load or
  save to the .ini file
- connect spline refresh to any bundle change
- connect spline widget refresh to spline refresh
- few caveat comments
- spline_widget::setConfig now only takes the spline as argument,
  eliminating most of the complex logic there
- clarify spline::set_bundle vs newly introduced spline::recompute

5 files changed, 218 insertions, 85 deletions

diff --git a/spline-widget/export.hpp b/spline-widget/export.hpp
new file mode 100644
index 00000000..4875b4c1
--- /dev/null
+++ b/spline-widget/export.hpp
@@ -0,0 +1,27 @@
+#pragma once
+
+#ifdef BUILD_spline_widget
+#   ifdef _WIN32
+#       define OPENTRACK_SPLINE_LINKAGE __declspec(dllexport)
+#   else
+#       define OPENTRACK_SPLINE_LINKAGE
+#   endif
+
+#   ifndef _MSC_VER
+#       define OPENTRACK_SPLINE_EXPORT __attribute__ ((visibility ("default"))) OPENTRACK_SPLINE_LINKAGE
+#   else
+#       define OPENTRACK_SPLINE_EXPORT OPENTRACK_SPLINE_LINKAGE
+#   endif
+#else
+    #ifdef _WIN32
+    #    define OPENTRACK_SPLINE_LINKAGE __declspec(dllimport)
+    #else
+    #    define OPENTRACK_SPLINE_LINKAGE
+    #endif
+
+    #ifndef _MSC_VER
+    #    define OPENTRACK_SPLINE_EXPORT __attribute__ ((visibility ("default"))) OPENTRACK_SPLINE_LINKAGE
+    #else
+    #    define OPENTRACK_SPLINE_EXPORT OPENTRACK_SPLINE_LINKAGE
+    #endif
+#endif
diff --git a/spline-widget/spline-widget.cpp b/spline-widget/spline-widget.cpp
index 85477ba7..ce16f7be 100644
--- a/spline-widget/spline-widget.cpp
+++ b/spline-widget/spline-widget.cpp
@@ -10,6 +10,8 @@
 #include <QPaintEvent>
 #include <QPen>
 #include <QPixmap>
+#include <QList>
+#include <QPointF>
 #include <cmath>
 #include <algorithm>
 
@@ -26,23 +28,27 @@ spline_widget::spline_widget(QWidget *parent) :
     setMouseTracking(true);
 }
 
-void spline_widget::setConfig(spline* config, bundle b)
+void spline_widget::setConfig(spline* spl)
 {
-    if (config && config->get_bundle() && config->get_bundle() != b)
+    if (spl != _config)
     {
-        QObject::disconnect(connection);
-        connection = QMetaObject::Connection();
-    }
+        if (connection)
+        {
+            QObject::disconnect(connection);
+            //connection = QMetaObject::Connection();
+        }
 
-    if (config && config->get_bundle() != b)
-        config->set_bundle(b);
-    _config = config;
-    if (b)
-        connection = connect(b.get(), &bundle_type::reloading,
-                             this, &spline_widget::reload_spline,
-                             Qt::QueuedConnection);
-    update_range();
-    update();
+        if (spl)
+        {
+            spline::settings& s = spl->get_settings();
+            connection = connect(&s, &spline::settings::recomputed,
+                                 this, [this]() { reload_spline(); },
+                                 Qt::QueuedConnection);
+        }
+
+        _config = spl;
+        update_range();
+    }
 }
 
 void spline_widget::set_preview_only(bool val)
@@ -417,9 +423,9 @@ void spline_widget::mouseReleaseEvent(QMouseEvent *e)
 
 void spline_widget::reload_spline()
 {
-    if (_config && _config->get_bundle() != nullptr)
+    if (_config)
     {
-        _config->set_bundle(_config->get_bundle());
+        // don't recompute here as the value's just been recomputed
         update_range();
     }
 }
diff --git a/spline-widget/spline-widget.hpp b/spline-widget/spline-widget.hpp
index 9ad4f598..5fc50504 100644
--- a/spline-widget/spline-widget.hpp
+++ b/spline-widget/spline-widget.hpp
@@ -12,14 +12,17 @@
 #include "spline.hpp"
 #include "api/plugin-api.hpp"
 #include "options/options.hpp"
+using namespace options;
+
+#include "export.hpp"
+
 #include <QWidget>
 #include <QtGui>
 #include <QMetaObject>
 #include <QPointF>
 #include <QDebug>
-using namespace options;
 
-class SPLINE_WIDGET_EXPORT spline_widget final : public QWidget
+class OPENTRACK_SPLINE_EXPORT spline_widget final : public QWidget
 {
     Q_OBJECT
     Q_PROPERTY(QColor colorBezier READ colorBezier WRITE setColorBezier)
@@ -31,7 +34,7 @@ public:
 
     spline* config();
 
-    void setConfig(spline* config, bundle b);
+    void setConfig(spline* spl);
 
     QColor colorBezier() const
     {
diff --git a/spline-widget/spline.cpp b/spline-widget/spline.cpp
index 8e5ef31b..a84f0d05 100644
--- a/spline-widget/spline.cpp
+++ b/spline-widget/spline.cpp
@@ -7,25 +7,55 @@
  */
 
 #include "spline.hpp"
+
+#include <algorithm>
+#include <cmath>
+#include <memory>
+
+#include <QObject>
 #include <QMutexLocker>
 #include <QCoreApplication>
 #include <QPointF>
 #include <QSettings>
 #include <QString>
-#include <algorithm>
-#include <cmath>
 
 #include <QDebug>
 
 constexpr int spline::value_count;
 
+spline::spline(qreal maxx, qreal maxy, const QString& name) :
+    s(nullptr),
+    data(value_count, -1.f),
+    _mutex(QMutex::Recursive),
+    max_x(maxx),
+    max_y(maxy),
+    activep(false)
+{
+    set_bundle(options::make_bundle(name));
+}
+
+spline::~spline()
+{
+    QMutexLocker l(&_mutex);
+
+    if (connection)
+    {
+        QObject::disconnect(connection);
+    }
+}
+
+spline::spline() : spline(0, 0, "")
+{
+}
+
 void spline::setTrackingActive(bool blnActive)
 {
     activep = blnActive;
 }
 
-spline::spline() : spline(0, 0, "")
+bundle spline::get_bundle()
 {
+    return s->b;
 }
 
 void spline::removeAllPoints()
@@ -39,12 +69,14 @@ void spline::setMaxInput(qreal max_input)
 {
     QMutexLocker l(&_mutex);
     max_x = max_input;
+    recompute();
 }
 
 void spline::setMaxOutput(qreal max_output)
 {
     QMutexLocker l(&_mutex);
     max_y = max_output;
+    recompute();
 }
 
 qreal spline::maxInput() const
@@ -59,17 +91,6 @@ qreal spline::maxOutput() const
     return max_y;
 }
 
-// todo try non-recursive mtx
-spline::spline(qreal maxx, qreal maxy, const QString& name) :
-    data(value_count, -1.f),
-    _mutex(QMutex::Recursive),
-    max_x(maxx),
-    max_y(maxy),
-    activep(false)
-{
-    set_bundle(options::make_bundle(name));
-}
-
 float spline::getValue(double x)
 {
     QMutexLocker foo(&_mutex);
@@ -131,7 +152,7 @@ void spline::update_interp_data()
     if (points.size() == 0)
         points.append(QPointF(max_x, max_y));
 
-    std::stable_sort(points.begin(), points.end(), sort_fn);
+    std::sort(points.begin(), points.end(), sort_fn);
 
     const double mult = precision(points);
     const double mult_ = mult * 30;
@@ -221,7 +242,7 @@ void spline::addPoint(QPointF pt)
 
     points_t points = s->points;
     points.push_back(pt);
-    std::stable_sort(points.begin(), points.end(), sort_fn);
+    std::sort(points.begin(), points.end(), sort_fn);
     s->points = points;
     update_interp_data();
 }
@@ -236,7 +257,7 @@ void spline::movePoint(int idx, QPointF pt)
     {
         points[idx] = pt;
         // we don't allow points to be reordered, but sort due to possible caller logic error
-        std::stable_sort(points.begin(), points.end(), sort_fn);
+        std::sort(points.begin(), points.end(), sort_fn);
         s->points = points;
         update_interp_data();
     }
@@ -250,14 +271,14 @@ QList<QPointF> spline::getPoints() const
 
 void spline::reload()
 {
-    if (s && s->b)
-        s->b->reload();
+    QMutexLocker foo(&_mutex);
+    s->b->reload();
 }
 
 void spline::save(QSettings& settings)
 {
-    if (s && s->b)
-        s->b->save_deferred(settings);
+    QMutexLocker foo(&_mutex);
+    s->b->save_deferred(settings);
 }
 
 void spline::save()
@@ -267,17 +288,55 @@ void spline::save()
 
 void spline::set_bundle(bundle b)
 {
-    QMutexLocker foo(&_mutex);
+    QMutexLocker l(&_mutex);
+
+    // gets called from ctor hence the need for nullptr checks
+    // the sentinel settings/bundle objects don't need any further branching once created
+    if (!s || s->b != b)
+    {
+        s = std::make_shared<settings>(b);
 
-    if (b && (!s || s->b != b))
-        s = ::make_unique<settings>(b);
-    else if (!b)
-        s = ::make_unique<settings>(nullptr);
+        if (connection)
+            QObject::disconnect(connection);
 
-    QList<QPointF> list = s->points;
+        if (b)
+        {
+            connection = QObject::connect(b.get(), &bundle_type::changed,
+                                          s.get(), [&]() {
+                                              // we're holding the mutex to allow signal disconnection in spline dtor
+                                              // before this slot gets called for the next time
+
+                                              // spline isn't a QObject and the connection context is incorrect
+
+                                              QMutexLocker l(&_mutex);
+                                              recompute();
+                                              emit s->recomputed();
+                                          },
+                                          Qt::QueuedConnection);
+        }
+
+        recompute();
+
+        emit s->recomputed();
+    }
+}
+
+void spline::recompute()
+{
+    QMutexLocker foo(&_mutex);
 
+    QList<QPointF> list = s->points;
     const int sz = list.size();
 
+    // storing to s->points fires bundle::changed and that leads to an infinite loop
+    // only store if we can't help it
+    std::sort(list.begin(), list.end(), sort_fn);
+
+    if (list != s->points)
+    {
+        s->points = list;
+    }
+
     QList<QPointF> ret_list;
     ret_list.reserve(sz);
 
@@ -285,15 +344,15 @@ void spline::set_bundle(bundle b)
     {
         QPointF& pt(list[i]);
 
-        pt = QPointF(clamp(pt.x(), 0, max_x),
-                     clamp(pt.y(), 0, max_y));
+        pt.setX(clamp(pt.x(), 0, max_x));
+        pt.setY(clamp(pt.y(), 0, max_y));
 
         const bool overlap = progn(
                                 for (int j = 0; j < i; j++)
                                 {
                                     QPointF& pt2(list[j]);
                                     const double dist_sq = (pt.x() - pt2.x())*(pt.x() - pt2.x());
-                                    if (dist_sq < .33)
+                                    if (dist_sq < .35)
                                     {
                                         return true;
                                     }
@@ -304,7 +363,8 @@ void spline::set_bundle(bundle b)
             ret_list.push_back(pt);
     }
 
-    s->points = ret_list;
+    if (ret_list != s->points)
+        s->points = ret_list;
 
     last_input_value = QPointF(0, 0);
     activep = false;
@@ -312,10 +372,33 @@ void spline::set_bundle(bundle b)
     update_interp_data();
 }
 
+// the return value is only safe to use with no spline::set_bundle calls
+spline::settings& spline::get_settings()
+{
+    QMutexLocker foo(&_mutex);
+    return *s;
+}
+
+const spline::settings& spline::get_settings() const
+{
+    QMutexLocker foo(&_mutex);
+    return *s;
+}
+
 int spline::precision(const QList<QPointF>& points) const
 {
+    // this adjusts the memoized range to the largest X value. empty space doesn't take value_count discrete points.
     if (points.size())
-        return int(value_count / clamp(points[points.size() - 1].x(), 1, max_x));
+        return clamp(value_count / clamp(int(points[points.size() - 1].x()), 1, int(max_x)), 0, value_count);
 
     return value_count / clamp(int(max_x), 1, value_count);
 }
+
+namespace spline_detail {
+
+settings::settings(bundle b):
+    b(b ? b : make_bundle("")),
+    points(b, "points", QList<QPointF>())
+{}
+
+}
diff --git a/spline-widget/spline.hpp b/spline-widget/spline.hpp
index 5b0b3afc..5d521a6c 100644
--- a/spline-widget/spline.hpp
+++ b/spline-widget/spline.hpp
@@ -8,43 +8,39 @@
 
 #pragma once
 
-#include <QObject>
-#include <QPointF>
-#include <QString>
-#include <QMutex>
-#include <vector>
-#include <limits>
-#include <memory>
 #include "compat/qcopyable-mutex.hpp"
 #include "options/options.hpp"
 using namespace options;
 
-#ifdef BUILD_spline_widget
-#   define SPLINE_WIDGET_EXPORT Q_DECL_EXPORT
-#else
-#   define SPLINE_WIDGET_EXPORT Q_DECL_IMPORT
-#endif
+#include "export.hpp"
 
-class SPLINE_WIDGET_EXPORT spline final
-{
-private:
-    struct settings
-    {
-        bundle b;
-        value<QList<QPointF>> points;
-        settings(bundle b) :
-            b(b),
-            points(b, "points", QList<QPointF>())
-        {}
-    };
+#include <vector>
+#include <limits>
+#include <memory>
 
-    ptr<settings> s;
+#include <QObject>
+#include <QPointF>
+#include <QString>
+#include <QMetaObject>
 
-    std::vector<float> data;
-    using interp_data_t = decltype(data);
+namespace spline_detail {
 
-    static constexpr int value_count = 10000;
+class OPENTRACK_SPLINE_EXPORT settings final : public QObject
+{
+    Q_OBJECT
+public:
+    bundle b;
+    value<QList<QPointF>> points;
+    settings(bundle b);
+signals:
+    void recomputed() const;
+};
+
+}
 
+class OPENTRACK_SPLINE_EXPORT spline final
+{
+private:
     int precision(const QList<QPointF>& points) const;
     void update_interp_data();
     float getValueInternal(int x);
@@ -53,11 +49,6 @@ private:
 
     static QPointF ensure_in_bounds(const QList<QPointF>& points, int i);
 
-    MyMutex _mutex;
-    QPointF last_input_value;
-    qreal max_x, max_y;
-    volatile bool activep;
-
     template<typename t, typename u, typename w>
     static inline auto clamp(t val, u min, w max) -> decltype (val * min * max)
     {
@@ -68,7 +59,22 @@ private:
         return val;
     }
 
+    mem<spline_detail::settings> s;
+    QMetaObject::Connection connection;
+
+    std::vector<float> data;
+    using interp_data_t = decltype(data);
+
+    static constexpr int value_count = 10000;
+
+    MyMutex _mutex;
+    QPointF last_input_value;
+    qreal max_x, max_y;
+    volatile bool activep;
+
 public:
+    using settings = spline_detail::settings;
+
     void reload();
     void save(QSettings& s);
     void save();
@@ -78,6 +84,10 @@ public:
     qreal maxOutput() const;
     spline();
     spline(qreal maxx, qreal maxy, const QString& name);
+    ~spline();
+
+    spline& operator=(const spline&) = default;
+    spline(const spline&) = default;
 
     float getValue(double x);
     bool getLastPoint(QPointF& point);
@@ -91,7 +101,11 @@ public:
     void setMaxOutput(qreal MaxOutput);
 
     void setTrackingActive(bool blnActive);
-    bundle get_bundle() { return s->b; }
+    bundle get_bundle();
+    void recompute();
+
+    settings& get_settings();
+    const settings& get_settings() const;
 
     using points_t = decltype(s->points.get());
 };