Launcher: fix a bug when using MP server 0 or 1

Due to how the popup menu was refresh, using MP servers at index 0 or 1
didn’t work correctly. Change some logic and add a helper to make this
work reliably, without a second Repeater and dummy properties.

src/GUI/CMakeLists.txt

@@ -164,6 +164,8 @@ if (HAVE_QT)
                         FlightPlanController.hxx
                         RouteDiagram.cxx
                         RouteDiagram.hxx
+                        ModelDataExtractor.cxx
+                        ModelDataExtractor.hxx
                         )
 
     set_property(TARGET fgqmlui PROPERTY AUTOMOC ON)

src/GUI/LauncherController.cxx

@@ -51,6 +51,7 @@
 #include "UnitsModel.hxx"
 #include "NavaidSearchModel.hxx"
 #include "FlightPlanController.hxx"
+#include "ModelDataExtractor.hxx"
 
 using namespace simgear::pkg;
 
@@ -164,6 +165,8 @@ void LauncherController::initQML()
     qmlRegisterType<RouteDiagram>("FlightGear", 1, 0, "RouteDiagram");
     qmlRegisterType<QmlRadioButtonGroup>("FlightGear", 1, 0, "RadioButtonGroup");
 
+    qmlRegisterType<ModelDataExtractor>("FlightGear", 1, 0, "ModelDataExtractor");
+
     qmlRegisterSingletonType(QUrl("qrc:/qml/OverlayShared.qml"), "FlightGear", 1, 0, "OverlayShared");
 
     QNetworkDiskCache* diskCache = new QNetworkDiskCache(this);

src/GUI/MPServersModel.cpp

@@ -140,6 +140,7 @@ void MPServersModel::onRefreshMPServersFailed(simgear::HTTP::Request*)
     m_servers.clear();
     endResetModel();
     emit validChanged();
+    emit currentIndexChanged(m_currentIndex);
     restoreMPServerSelection();
 }
 
@@ -150,19 +151,21 @@ void MPServersModel::restoreMPServerSelection()
         settings.beginGroup("mpSettings");
         QString host = settings.value("mp-server").toString();
         if (host == "__custom__") {
-            emit restoreIndex(m_servers.size());
+            setCurrentIndex(m_servers.size());
         } else {
             // restore a built-in server
             auto it = std::find_if(m_servers.begin(), m_servers.end(), [host](const ServerInfo& info)
             { return (info.host == host); });
 
             if (it != m_servers.end()) {
-                emit restoreIndex(std::distance(m_servers.begin(), it));
+                setCurrentIndex(std::distance(m_servers.begin(), it));
             } else {
-                emit restoreDefault();
+                setCurrentIndex(0);
             }
         }
 
+        // force a refresh on the QML side, since the model may have changed
+        emit currentIndexChanged(m_currentIndex);
         m_doRestoreMPServer = false;
     }
 }
@@ -172,33 +175,25 @@ void MPServersModel::requestRestore()
     m_doRestoreMPServer = true;
 }
 
-QString MPServersModel::serverForIndex(int index) const
+QString MPServersModel::currentServer() const
 {
     if (!valid()) {
-        return (index == 1) ? "__custom__" : "__noservers__";
+        return (m_currentIndex == 1) ? "__custom__" : "__noservers__";
     }
 
-    if ((index < 0) || (index > m_servers.size())) {
-        return QString();
-    }
-
-    if (index == m_servers.size()) {
+    if (m_currentIndex == m_servers.size()) {
         return "__custom__";
     }
 
-    return m_servers.at(index).host;
+    return m_servers.at(m_currentIndex).host;
 }
 
-int MPServersModel::portForIndex(int index) const
+int MPServersModel::currentPort() const
 {
     if (!valid())
         return 0;
 
-    if ((index < 0) || (index >= m_servers.size())) {
-        return 0;
-    }
-
-    return m_servers.at(index).port;
+    return m_servers.at(m_currentIndex).port;
 }
 
 bool MPServersModel::valid() const
@@ -206,6 +201,15 @@ bool MPServersModel::valid() const
     return !m_servers.empty();
 }
 
+void MPServersModel::setCurrentIndex(int currentIndex)
+{
+    if (m_currentIndex == currentIndex)
+        return;
+
+    m_currentIndex = currentIndex;
+    emit currentIndexChanged(m_currentIndex);
+}
+
 MPServersModel::ServerInfo::ServerInfo(QString n, QString l, QString h, int p)
 {
     name = n;

src/GUI/MPServersModel.h

@@ -11,9 +11,14 @@ class MPServersModel : public QAbstractListModel
     Q_OBJECT
 
     Q_PROPERTY(bool valid READ valid NOTIFY validChanged)
+
+    Q_PROPERTY(int currentIndex READ currentIndex WRITE setCurrentIndex NOTIFY currentIndexChanged)
+
+    Q_PROPERTY(QString currentServer READ currentServer NOTIFY currentIndexChanged)
+    Q_PROPERTY(int currentPort READ currentPort NOTIFY currentIndexChanged)
 public:
     MPServersModel(QObject* parent = nullptr);
-    ~MPServersModel();
+    ~MPServersModel() override;
 
     int rowCount(const QModelIndex& index) const override;
 
@@ -30,15 +35,22 @@ public:
 
     void requestRestore();
 
-    Q_INVOKABLE QString serverForIndex(int index) const;
-    Q_INVOKABLE int portForIndex(int index) const;
+    QString currentServer() const;
+    int currentPort() const;
 
     bool valid() const;
 
+    int currentIndex() const
+    {
+        return m_currentIndex;
+    }
+
+public slots:
+    void setCurrentIndex(int currentIndex);
+
 signals:
-    void restoreIndex(int index);
-    void restoreDefault();
     void validChanged();
+    void currentIndexChanged(int currentIndex);
 
 private:
 
@@ -54,6 +66,7 @@ private:
     };
 
     std::vector<ServerInfo> m_servers;
+    int m_currentIndex = 0;
 };
 
 #endif // MPSERVERSMODEL_H

src/GUI/ModelDataExtractor.cxx

@@ -0,0 +1,88 @@
+#include "ModelDataExtractor.hxx"
+
+#include <QAbstractItemModel>
+
+ModelDataExtractor::ModelDataExtractor(QObject *parent) : QObject(parent)
+{
+
+}
+
+QVariant ModelDataExtractor::data() const
+{
+    if (m_model) {
+        QModelIndex m = m_model->index(m_index, 0);
+        if (!m.isValid())
+            return {};
+
+        int role = Qt::DisplayRole;
+        if (!m_role.isEmpty()) {
+            const auto& names = m_model->roleNames();
+            role = names.key(m_role.toUtf8(), Qt::DisplayRole);
+        }
+
+        return m_model->data(m, role);
+    }
+
+    if (m_value.isArray()) {
+        return m_value.property(m_index).toVariant();
+    }
+
+    return {};
+}
+
+void ModelDataExtractor::setModel(QJSValue model)
+{
+    if (m_value.equals(model))
+        return;
+
+    if (m_model) {
+        // disconnect from everything
+        disconnect(m_model, nullptr, this, nullptr);
+        m_model = nullptr;
+    }
+
+    m_value = model;
+    if (m_value.isQObject()) {
+        m_model = qobject_cast<QAbstractItemModel*>(m_value.toQObject());
+        if (m_model) {
+            connect(m_model, &QAbstractItemModel::modelReset,
+                    this, &ModelDataExtractor::dataChanged);
+            connect(m_model, &QAbstractItemModel::dataChanged,
+                    this, &ModelDataExtractor::onDataChanged);
+
+            // ToDo: handle rows added / removed
+        }
+    } else {
+        // might be null, or an array
+    }
+
+    emit modelChanged();
+    emit dataChanged();
+}
+
+void ModelDataExtractor::setIndex(int index)
+{
+    if (m_index == index)
+        return;
+
+    m_index = index;
+    emit indexChanged(m_index);
+    emit dataChanged();
+}
+
+void ModelDataExtractor::setRole(QString role)
+{
+    if (m_role == role)
+        return;
+
+    m_role = role;
+    emit roleChanged(m_role);
+    emit dataChanged();
+}
+
+void ModelDataExtractor::onDataChanged(const QModelIndex &topLeft, const QModelIndex &bottomRight)
+{
+    if ((topLeft.row() >= m_index) && (bottomRight.row() <= m_index)) {
+        emit dataChanged();
+    }
+}

src/GUI/ModelDataExtractor.hxx

@@ -0,0 +1,61 @@
+#ifndef MODELDATAEXTRACTOR_HXX
+#define MODELDATAEXTRACTOR_HXX
+
+#include <QObject>
+#include <QJSValue>
+
+class QAbstractItemModel;
+
+class ModelDataExtractor : public QObject
+{
+    Q_OBJECT
+public:
+    explicit ModelDataExtractor(QObject *parent = nullptr);
+
+    Q_PROPERTY(QJSValue model READ model WRITE setModel NOTIFY modelChanged)
+    Q_PROPERTY(int index READ index WRITE setIndex NOTIFY indexChanged)
+    Q_PROPERTY(QString role READ role WRITE setRole NOTIFY roleChanged)
+
+    Q_PROPERTY(QVariant data READ data NOTIFY dataChanged)
+
+    QJSValue model() const
+    {
+        return m_value;
+    }
+
+    int index() const
+    {
+        return m_index;
+    }
+
+    QString role() const
+    {
+        return m_role;
+    }
+
+    QVariant data() const;
+
+signals:
+
+    void modelChanged();
+    void indexChanged(int index);
+    void roleChanged(QString role);
+
+    void dataChanged();
+
+public slots:
+    void setModel(QJSValue model);
+    void setIndex(int index);
+    void setRole(QString role);
+
+private slots:
+    void onDataChanged(const QModelIndex& topLeft, const QModelIndex& bottomRight);
+
+private:
+    QAbstractItemModel* m_model = nullptr;
+    QJSValue m_value;
+    int m_index = 0;
+    QString m_role;
+};
+
+#endif // MODELDATAEXTRACTOR_HXX

src/GUI/qml/PopupChoice.qml

@@ -7,11 +7,10 @@ Item {
     id: root
     property alias label: label.text
 
-    property var model: undefined
+    property var model: nil
     property string displayRole: "display"
     property bool enabled: true
     property int currentIndex: 0
-    property bool __dummy: false
     property string headerText: ""
 
     implicitHeight: Math.max(label.implicitHeight, currentChoiceFrame.height)
@@ -22,32 +21,11 @@ Item {
         root.currentIndex = index;
     }
 
-    Item {
-        Repeater {
-            id: internalModel
-            model: root.model
-
-            Item {
-                id: internalModelItem
-
-                // Taken from TableViewItemDelegateLoader.qml to follow QML role conventions
-                readonly property var text: model && model.hasOwnProperty(displayRole) ? model[displayRole] // Qml ListModel and QAbstractItemModel
-                                                                                       : modelData && modelData.hasOwnProperty(displayRole) ? modelData[displayRole] // QObjectList / QObject
-                                                                                                                                            : modelData != undefined ? modelData : "" // Models without role
-                readonly property bool selected: root.currentIndex === model.index
-                readonly property QtObject modelObj: model
-            }
-        }
-    }
-
-    Component.onCompleted: {
-        // hack to force updating of currentText after internalModel
-        // has been populated
-        __dummy = !__dummy
-    }
-
-    onModelChanged: {
-         __dummy = !__dummy // force update of currentText
+    ModelDataExtractor {
+        id: currentItemText
+        model: root.model
+        role: root.displayRole
+        index: root.currentIndex
     }
 
     function haveHeader()
@@ -60,10 +38,7 @@ Item {
         if ((currentIndex == -1) && haveHeader())
             return headerText;
 
-        var foo = __dummy; // fake propery dependency to update this
-        var item = internalModel.itemAt(currentIndex);
-        if (!item) return "";
-        return item.text
+        return currentItemText.data
     }
 
     StyledText {

src/GUI/qml/Settings.qml

@@ -179,9 +179,8 @@ Item {
                         description: qsTr("Select a server close to you for better responsiveness and reduced lag when flying online.")
                         choices: _launcher.mpServersModel
 
-                        readonly property bool currentIsCustom: (_launcher.mpServersModel.serverForIndex(selectedIndex) === "__custom__")
-                        readonly property bool currentIsNoServers: (_launcher.mpServersModel.serverForIndex(selectedIndex) === "__noservers__")
-
+                        readonly property bool currentIsCustom: (_launcher.mpServersModel.currentServer === "__custom__")
+                        readonly property bool currentIsNoServers: (_launcher.mpServersModel.currentServer === "__noservers__")
                         property string __savedServer;
 
                         keywords: ["server", "hostname"]
@@ -195,7 +194,7 @@ Item {
                         {
                             // these values match the code in MPServersModel.cpp, sorry for that
                             // nastyness
-                            _config.setValueForKey("mpSettings", "mp-server", _launcher.mpServersModel.serverForIndex(selectedIndex) );
+                            _config.setValueForKey("mpSettings", "mp-server", _launcher.mpServersModel.currentServer);
                         }
 
                         function restoreState()
@@ -203,11 +202,13 @@ Item {
                             // no-op, this is triggered by MPServersModel::restoreMPServerSelection
                         }
 
+                        // can't use a Binding here, since we need a bidrectional link
+                        onSelectedIndexChanged: _launcher.mpServersModel.currentIndex = selectedIndex
+
                         Connections
                         {
                             target: _launcher.mpServersModel
-                            onRestoreIndex: { mpServer.selectedIndex = index }
-                            onRestoreDefault: { mpServer.selectedIndex = 0; }
+                            onCurrentIndexChanged: mpServer.selectedIndex = _launcher.mpServersModel.currentIndex
                         }
                     },
 
@@ -229,26 +230,24 @@ Item {
                             console.warn("MP enabled but no valid server selected, skipping");
                         } else if (mpServer.currentIsCustom) {
                             var pieces = mpCustomServer.value.split(':')
-                            var port = defaultMPPort;
+                            var customPort = defaultMPPort;
                             if (pieces.length > 1) {
-                                port = pieces[1];
+                                customPort = pieces[1];
                             }
 
                             if (pieces[0].length > 0) {
                                 _config.setProperty("/sim/multiplay/txhost", pieces[0]);
-                                _config.setProperty("/sim/multiplay/txport", port);
+                                _config.setProperty("/sim/multiplay/txport", customPort);
                             } else {
                                 console.warn("Custom MP server selected but no hostname defined");
                             }
                         } else {
-                            var sel = mpServer.selectedIndex
-                            var host =  _launcher.mpServersModel.serverForIndex(sel);
-                            console.info("MP host is " + host)
+                            var host =  _launcher.mpServersModel.currentServer;
                             if (host.length > 0) {
                                 _config.setProperty("/sim/multiplay/txhost", host);
                             }
 
-                            var port = _launcher.mpServersModel.portForIndex(sel);
+                            var port = _launcher.mpServersModel.currentPort;
                             if (port === 0) {
                                 console.info("Using default MP port");
                                 port = defaultMPPort;