Fixes for the Player interface testing code

6 files changed, 109 insertions, 37 deletions

diff --git a/metadatamodel.cpp b/metadatamodel.cpp
index 3484ad7..513e3d0 100644
--- a/metadatamodel.cpp
+++ b/metadatamodel.cpp
@@ -15,6 +15,8 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 #include "metadatamodel.h"
+#include <QDBusArgument>
+#include <qdebug.h>
 
 MetadataModel::MetadataModel(QObject* parent)
     : QAbstractTableModel(parent)
@@ -49,7 +51,14 @@ QVariant MetadataModel::data(const QModelIndex& index, int role) const
             if (index.column() == 0) {
                 return key;
             } else {
-                return m_metadata[key];
+                if (m_metadata[key].canConvert<QDBusArgument>()) {
+                    qDebug() << "Unmarshalled type in metadata map for entry" << key;
+                    return QVariant();
+                } else if (m_metadata[key].canConvert<QDBusObjectPath>()) {
+                    return m_metadata[key].value<QDBusObjectPath>().path();
+                } else {
+                    return m_metadata[key];
+                }
             }
         }
     }
diff --git a/mpris2/interfacetest.cpp b/mpris2/interfacetest.cpp
index 8422921..11fdd84 100644
--- a/mpris2/interfacetest.cpp
+++ b/mpris2/interfacetest.cpp
@@ -135,11 +135,15 @@ bool InterfaceTest::checkPropValid(const QString& propName, QVariant::Type expTy
         return false;
     } else if (oldProps.contains(propName)) {
         // FIXME: QVariant equality only works for builtin types
-        if (props[propName] != oldProps[propName]) {
-            outOfDateProperties.insert(propName, props[propName]);
-            props[propName] = oldProps[propName];
-            // don't check right now
-            return false;
+        if (props[propName].type() < QVariant::UserType) {
+            if (props[propName] != oldProps[propName]) {
+                outOfDateProperties.insert(propName, props[propName]);
+                props[propName] = oldProps[propName];
+                // don't check right now
+                return false;
+            }
+        } else {
+            qDebug() << "Could not check equality for" << propName;
         }
     }
     return true;
@@ -167,7 +171,7 @@ void InterfaceTest::initialTest()
             iface->service(),
             iface->path(),
             DBUS_PROPS_IFACE,
-            "propertiesChanged", /* signature, */
+            "PropertiesChanged", /* signature, */
             this,
             SLOT( _m_propertiesChanged(QString,QVariantMap,QStringList,QDBusMessage)));
     connectSignals();
diff --git a/mpris2/interfacetest.h b/mpris2/interfacetest.h
index 8d62b2c..cfc174c 100644
--- a/mpris2/interfacetest.h
+++ b/mpris2/interfacetest.h
@@ -25,6 +25,7 @@ class QDBusInterface;
 class QDBusMessage;
 class QTimer;
 #include <QObject>
+#include <QStringList>
 #include <QVariantMap>
 
 namespace Mpris2
@@ -130,6 +131,7 @@ namespace Mpris2
         QDBusInterface* iface;
         QVariantMap     props;
         QVariantMap     outOfDateProperties; // prop name -> new value
+        QStringList     propsNotUpdated;
 
     private:
         QDBusInterface*    propsIface;
diff --git a/mpris2/playerinterfacetest.cpp b/mpris2/playerinterfacetest.cpp
index d663809..88e55cf 100644
--- a/mpris2/playerinterfacetest.cpp
+++ b/mpris2/playerinterfacetest.cpp
@@ -29,6 +29,7 @@ PlayerInterfaceTest::PlayerInterfaceTest(const QString& service, QObject* parent
 {
     m_pos = -1;
     m_currentRate = 0.0;
+    propsNotUpdated << "Position";
 }
 
 PlayerInterfaceTest::~PlayerInterfaceTest()
@@ -81,6 +82,10 @@ void PlayerInterfaceTest::checkUpdatedProperty(const QString& propName)
 
 void PlayerInterfaceTest::checkProps(const QVariantMap& oldProps)
 {
+    // position is time-sensitive; check it first
+    checkPosition(oldProps);
+    checkPredictedPosition();
+
     checkPropValid("CanControl", QVariant::Bool, oldProps);
     checkControlProp("CanGoNext", oldProps);
     checkControlProp("CanGoPrevious", oldProps);
@@ -105,7 +110,6 @@ void PlayerInterfaceTest::checkProps(const QVariantMap& oldProps)
     checkMaximumRate(oldProps);
     checkPropValid("Rate", QVariant::Double, oldProps);
     checkMetadata(oldProps);
-    checkPosition(oldProps);
 
     checkConsistency();
 }
@@ -156,9 +160,9 @@ void PlayerInterfaceTest::checkPlaybackStatus(const QVariantMap& oldProps)
     if (!checkPropValid("PlaybackStatus", QVariant::String, oldProps))
         return;
     QString playbackStatus = properties().value("PlaybackStatus").toString();
-    if (playbackStatus != "None" &&
-        playbackStatus != "Track" &&
-        playbackStatus != "Playlist")
+    if (playbackStatus != "Playing" &&
+        playbackStatus != "Paused" &&
+        playbackStatus != "Stopped")
     {
         emit interfaceError(Property, "PlaybackStatus",
                             "Invalid value: '" + playbackStatus + "'");
@@ -202,31 +206,68 @@ void PlayerInterfaceTest::checkRate(const QVariantMap& oldProps)
     }
 }
 
+static bool compare(const QVariantMap& one, const QVariantMap& other)
+{
+    if (one.size() != other.size())
+        return false;
+
+    QVariantMap::const_iterator it1 = one.begin();
+    QVariantMap::const_iterator it2 = other.begin();
+
+    while (it1 != one.end()) {
+        if (it1.value().userType() != it2.value().userType())
+            return false;
+        if (!(it1.value() == it2.value())) {
+            if (it1.value().userType() == qMetaTypeId<QDBusObjectPath>()) {
+                if (!(it1.value().value<QDBusObjectPath>() == it2.value().value<QDBusObjectPath>()))
+                    return false;
+            }
+        }
+        if (qMapLessThanKey(it1.key(), it2.key()) || qMapLessThanKey(it2.key(), it1.key()))
+            return false;
+        ++it2;
+        ++it1;
+    }
+    return true;
+}
+
 void PlayerInterfaceTest::checkMetadata(const QVariantMap& oldProps)
 {
-    if (!properties().contains("Metadata")) {
+    if (!props.contains("Metadata")) {
         emit interfaceError(Property, "Metadata", "Property Metadata is missing");
         return;
     }
-    if (!properties().value("Metadata").canConvert<QDBusArgument>()) {
+    if (props["Metadata"].type() != QVariant::Map &&
+        !props.value("Metadata").canConvert<QDBusArgument>())
+    {
         const char * gotTypeCh = QDBusMetaType::typeToSignature(props["Metadata"].userType());
         QString gotType = gotTypeCh ? QString::fromAscii(gotTypeCh) : "<unknown>";
         emit interfaceError(Property, "Metadata", "Property Metadata has type " + gotType + ", but should be type a{sv}");
         return;
     }
     QVariantMap metadata;
-    QDBusArgument arg = properties().value("Metadata").value<QDBusArgument>();
-    arg >> metadata;
+    if (props["Metadata"].type() == QVariant::Map) {
+        metadata = props["Metadata"].toMap();
+    } else {
+        QDBusArgument arg = props["Metadata"].value<QDBusArgument>();
+        arg >> metadata;
+        // replace the entry in the properties array
+        props["Metadata"] = metadata;
+    }
 
     if (oldProps.contains("Metadata") &&
-        oldProps.value("Metadata").canConvert<QDBusArgument>())
+        oldProps.value("Metadata").canConvert(QVariant::Map))
     {
-        QVariantMap oldMetadata;
-        oldProps.value("Metadata").value<QDBusArgument>() >> oldMetadata;
-        if (metadata != oldMetadata) {
-            outOfDateProperties.insert("Metadata", props["Metadata"]);
+        QVariantMap oldMetadata = oldProps.value("Metadata").toMap();
+
+        // custom compare fn as we're expecting a QDBusObjectPath entry
+        if (!compare(metadata, oldMetadata)) {
+            outOfDateProperties["Metadata"] = props["Metadata"];
             props["Metadata"] = oldProps["Metadata"];
             return;
+        } else {
+            // same as before; don't re-run checks
+            return;
         }
     }
     if (metadata.isEmpty()) {
@@ -240,8 +281,8 @@ void PlayerInterfaceTest::checkMetadata(const QVariantMap& oldProps)
                             "No mpris:trackid entry for the current track");
     } else if (metadata.value("mpris:trackid").userType() != qMetaTypeId<QDBusObjectPath>()) {
         emit interfaceError(Property, "Metadata",
-                            "mpris:trackid entry is not a D-Bus object path");
-    } else if (metadata.value("mpris:trackid").toString().isEmpty()) {
+                            "mpris:trackid entry was not sent as a D-Bus object path (D-Bus type 'o')");
+    } else if (metadata.value("mpris:trackid").value<QDBusObjectPath>().path().isEmpty()) {
         emit interfaceError(Property, "Metadata",
                             "mpris:trackid entry is an empty path");
     }
@@ -261,7 +302,7 @@ void PlayerInterfaceTest::checkMetadata(const QVariantMap& oldProps)
                                         "mpris:artUrl references a file that does not exist");
                 }
             }
-            // FIXME: check network files
+            // TODO: check network files
         }
     }
 
@@ -389,7 +430,6 @@ void PlayerInterfaceTest::checkConsistency(const QVariantMap& oldProps)
 {
     checkRateConsistency();
     checkPositionConsistency();
-    checkPredictedPosition();
 }
 
 void PlayerInterfaceTest::checkPositionConsistency(const QVariantMap& oldProps)
@@ -511,23 +551,33 @@ void PlayerInterfaceTest::checkPredictedPosition()
     m_posLastUpdated = QTime::currentTime();
     updateCurrentRate();
 
+    qint64 diffMillis = (position - predictedPos) / 1000;
     // allow 1s of error
-    const qint64 allowance = 1000000;
-    if (position - predictedPos > allowance ||
-        position - predictedPos < -allowance)
+    const qint64 allowanceMillis = 1000;
+    if (diffMillis > allowanceMillis ||
+        diffMillis < -allowanceMillis)
     {
-        qint64 diffMillis = (position - predictedPos) / 1000;
-        qreal diffSecs = (qreal)diffMillis / 1000.0;
+        qreal diffSecs = ((qreal)diffMillis) / 1000.0;
+        qreal predictedPosSecs = ((qreal)(predictedPos/1000)) / 1000.0;
+        qreal positionSecs = ((qreal)(position/1000)) / 1000.0;
         if (diffMillis > 0) {
             emit interfaceWarning(Property, "Position",
-                                  "Position is " +
-                                  QString::number(diffSecs, 'g', 2) +
-                                  "ms ahead of what was predicted from Rate");
+                                  "Position (" +
+                                  QString::number(positionSecs, 'f', 2) +
+                                  "s) is " +
+                                  QString::number(diffSecs, 'f', 2) +
+                                  "s ahead of what was predicted from Rate (" +
+                                  QString::number(predictedPosSecs, 'f', 2) +
+                                  "s)");
         } else {
             emit interfaceWarning(Property, "Position",
-                                  "Position is " + QString::number(-diffMillis) +
-                                  QString::number(-diffSecs, 'g', 2) +
-                                  "ms behind of what was predicted from Rate");
+                                  "Position (" +
+                                  QString::number(positionSecs, 'f', 2) +
+                                  "s) is " +
+                                  QString::number(-diffSecs, 'f', 2) +
+                                  "s ahead of what was predicted from Rate (" +
+                                  QString::number(predictedPosSecs, 'f', 2) +
+                                  "s)");
         }
     }
 }
diff --git a/mpris2/playertestwidget.cpp b/mpris2/playertestwidget.cpp
index 8ed42fe..c35ee5c 100644
--- a/mpris2/playertestwidget.cpp
+++ b/mpris2/playertestwidget.cpp
@@ -20,8 +20,9 @@
 
 #include "playertestwidget.h"
 #include "playerinterfacetest.h"
-#include <qtimer.h>
-#include <qdbusextratypes.h>
+#include <qdebug.h>
+#include <QTimer>
+#include <QDBusObjectPath>
 
 using namespace Mpris2;
 
@@ -141,7 +142,11 @@ void PlayerTestWidget::propertiesChanged(const QStringList& properties)
             estPosTimer->start();
     }
     if (test->properties().contains("Metadata")) {
+        if (test->properties().value("Metadata").type() != QVariant::Map) {
+            qDebug() << "Metadata map was wrong type";
+        }
         metadataModel->setMetadata(test->properties().value("Metadata").toMap());
+        ui.metadataTableView->setEnabled(true);
     }
 }
 
diff --git a/window.cpp b/window.cpp
index 368d80a..7aed710 100644
--- a/window.cpp
+++ b/window.cpp
@@ -138,6 +138,8 @@ void Window::setPlayer(const QString& dbusAddress)
 {
     clear();
 
+    m_currentPlayer = dbusAddress;
+
     qDebug() << "Connecting to player" << dbusAddress;
 
     m_tabWidget = new QTabWidget(this);