Fix enabling polymorphic pimpl in QGeoRoute/Segment/Maneuver

Change 8ac6377e62af803b567449cdf30c669b92114cc4 allowed to subclass
private implementations of these 3 classes.
However, the pimpl was kept in a QExplicitlySharedDataPointer for
the first two, and QSharedDataPointer for the last class.

Failing to specialize the clone method of the pointer would result in
incorrect detach behavior, which would however only be visible with the
third class (the first two never call detach() internally, and the line
QEXPECT_FAIL("", "QGeoRoute equality operators broken", Continue); in
the tests make it clear.

This change also adds the equals virtual method also to QGeoRouteSegment
and QGeoManeuver private implementations.

Change-Id: If51dafebb19e4cb2d1bcf40c2b6f136f804198e0
Reviewed-by: Alex Blasche <alexander.blasche@qt.io>

13 files changed, 130 insertions, 14 deletions

diff --git a/src/location/maps/qgeomaneuver.cpp b/src/location/maps/qgeomaneuver.cpp
index 78efb971..9d6a4bf0 100644
--- a/src/location/maps/qgeomaneuver.cpp
+++ b/src/location/maps/qgeomaneuver.cpp
@@ -41,6 +41,12 @@
 
 QT_BEGIN_NAMESPACE
 
+template<>
+QGeoManeuverPrivate *QSharedDataPointer<QGeoManeuverPrivate>::clone()
+{
+    return d->clone();
+}
+
 /*!
     \class QGeoManeuver
     \inmodule QtLocation
@@ -146,7 +152,8 @@ QGeoManeuver &QGeoManeuver::operator= (const QGeoManeuver & other)
 */
 bool QGeoManeuver::operator== (const QGeoManeuver &other) const
 {
-    return (*(d_ptr.constData()) == *(other.d_ptr.constData()));
+    return ( (d_ptr.constData() == other.d_ptr.constData())
+            ||  (*(d_ptr.constData()) == *(other.d_ptr.constData())) );
 }
 
 /*!
@@ -308,6 +315,11 @@ QGeoManeuverPrivate::~QGeoManeuverPrivate()
 
 bool QGeoManeuverPrivate::operator==(const QGeoManeuverPrivate &other) const
 {
+    return equals(other);
+}
+
+bool QGeoManeuverPrivate::equals(const QGeoManeuverPrivate &other) const
+{
     return ((valid() == other.valid())
             && (position() == other.position())
             && (text() == other.text())
@@ -420,11 +432,9 @@ QGeoManeuverPrivateDefault::QGeoManeuverPrivateDefault(const QGeoManeuverPrivate
 
 QGeoManeuverPrivateDefault::~QGeoManeuverPrivateDefault() {}
 
-
-
-bool QGeoManeuverPrivateDefault::operator ==(const QGeoManeuverPrivateDefault &other) const
+QGeoManeuverPrivate *QGeoManeuverPrivateDefault::clone()
 {
-    return QGeoManeuverPrivateDefault::operator ==(other);
+    return new QGeoManeuverPrivateDefault(*this);
 }
 
 bool QGeoManeuverPrivateDefault::valid() const
diff --git a/src/location/maps/qgeomaneuver_p.h b/src/location/maps/qgeomaneuver_p.h
index 484354ba..e17bf880 100644
--- a/src/location/maps/qgeomaneuver_p.h
+++ b/src/location/maps/qgeomaneuver_p.h
@@ -63,6 +63,7 @@ public:
     QGeoManeuverPrivate();
     QGeoManeuverPrivate(const QGeoManeuverPrivate &other);
     virtual ~QGeoManeuverPrivate();
+    virtual QGeoManeuverPrivate *clone() = 0;
 
     bool operator== (const QGeoManeuverPrivate &other) const;
 
@@ -90,6 +91,8 @@ public:
     virtual QGeoCoordinate waypoint() const;
     virtual void setWaypoint(const QGeoCoordinate &waypoint);
 
+protected:
+    virtual bool equals(const QGeoManeuverPrivate &other) const;
 };
 
 class Q_LOCATION_PRIVATE_EXPORT QGeoManeuverPrivateDefault : public QGeoManeuverPrivate
@@ -98,8 +101,7 @@ public:
     QGeoManeuverPrivateDefault();
     QGeoManeuverPrivateDefault(const QGeoManeuverPrivateDefault &other);
     ~QGeoManeuverPrivateDefault();
-
-    bool operator== (const QGeoManeuverPrivateDefault &other) const;
+    virtual QGeoManeuverPrivate *clone() override;
 
     virtual bool valid() const override;
     virtual void setValid(bool valid) override;
diff --git a/src/location/maps/qgeoroute.cpp b/src/location/maps/qgeoroute.cpp
index 95603218..2e7b259a 100644
--- a/src/location/maps/qgeoroute.cpp
+++ b/src/location/maps/qgeoroute.cpp
@@ -44,6 +44,12 @@
 
 QT_BEGIN_NAMESPACE
 
+template<>
+QGeoRoutePrivate *QExplicitlySharedDataPointer<QGeoRoutePrivate>::clone()
+{
+    return d->clone();
+}
+
 /*!
     \class QGeoRoute
     \inmodule QtLocation
@@ -434,6 +440,11 @@ QGeoRoutePrivateDefault::QGeoRoutePrivateDefault(const QGeoRoutePrivateDefault &
 
 QGeoRoutePrivateDefault::~QGeoRoutePrivateDefault() {}
 
+QGeoRoutePrivate *QGeoRoutePrivateDefault::clone()
+{
+    return new QGeoRoutePrivateDefault(*this);
+}
+
 void QGeoRoutePrivateDefault::setId(const QString &id)
 {
     m_id = id;
diff --git a/src/location/maps/qgeoroute_p.h b/src/location/maps/qgeoroute_p.h
index 1a8f9b0c..26eb613c 100644
--- a/src/location/maps/qgeoroute_p.h
+++ b/src/location/maps/qgeoroute_p.h
@@ -66,6 +66,7 @@ public:
     QGeoRoutePrivate();
     QGeoRoutePrivate(const QGeoRoutePrivate &other);
     virtual ~QGeoRoutePrivate();
+    virtual QGeoRoutePrivate *clone() = 0;
 
     bool operator == (const QGeoRoutePrivate &other) const;
 
@@ -93,10 +94,11 @@ public:
     virtual void setFirstSegment(const QGeoRouteSegment &firstSegment);
     virtual QGeoRouteSegment firstSegment() const;
 
-    virtual bool equals(const QGeoRoutePrivate &other) const;
-
     virtual QString engineName() const = 0;
     virtual int segmentsCount() const = 0;
+
+protected:
+    virtual bool equals(const QGeoRoutePrivate &other) const;
 };
 
 class Q_LOCATION_PRIVATE_EXPORT  QGeoRoutePrivateDefault : public QGeoRoutePrivate
@@ -105,6 +107,7 @@ public:
     QGeoRoutePrivateDefault();
     QGeoRoutePrivateDefault(const QGeoRoutePrivateDefault &other);
     ~QGeoRoutePrivateDefault();
+    virtual QGeoRoutePrivate *clone() override;
 
     virtual void setId(const QString &id) override;
     virtual QString id() const override;
diff --git a/src/location/maps/qgeoroutesegment.cpp b/src/location/maps/qgeoroutesegment.cpp
index d9de9870..8dbe18fc 100644
--- a/src/location/maps/qgeoroutesegment.cpp
+++ b/src/location/maps/qgeoroutesegment.cpp
@@ -42,6 +42,12 @@
 
 QT_BEGIN_NAMESPACE
 
+template<>
+QGeoRouteSegmentPrivate *QExplicitlySharedDataPointer<QGeoRouteSegmentPrivate>::clone()
+{
+    return d->clone();
+}
+
 /*!
     \class QGeoRouteSegment
     \inmodule QtLocation
@@ -250,6 +256,11 @@ QGeoRouteSegmentPrivate::~QGeoRouteSegmentPrivate()
 
 bool QGeoRouteSegmentPrivate::operator ==(const QGeoRouteSegmentPrivate &other) const
 {
+    return equals(other);
+}
+
+bool QGeoRouteSegmentPrivate::equals(const QGeoRouteSegmentPrivate &other) const
+{
     return ((valid() == other.valid())
             && (travelTime() == other.travelTime())
             && (distance() == other.distance())
@@ -344,6 +355,11 @@ QGeoRouteSegmentPrivateDefault::~QGeoRouteSegmentPrivateDefault()
 
 }
 
+QGeoRouteSegmentPrivate *QGeoRouteSegmentPrivateDefault::clone()
+{
+    return new QGeoRouteSegmentPrivateDefault(*this);
+}
+
 bool QGeoRouteSegmentPrivateDefault::operator ==(const QGeoRouteSegmentPrivateDefault &other) const
 {
     return QGeoRouteSegmentPrivateDefault::operator ==(other);
diff --git a/src/location/maps/qgeoroutesegment_p.h b/src/location/maps/qgeoroutesegment_p.h
index 8f452896..f0b180da 100644
--- a/src/location/maps/qgeoroutesegment_p.h
+++ b/src/location/maps/qgeoroutesegment_p.h
@@ -67,6 +67,7 @@ public:
     QGeoRouteSegmentPrivate();
     QGeoRouteSegmentPrivate(const QGeoRouteSegmentPrivate &other);
     virtual ~QGeoRouteSegmentPrivate();
+    virtual QGeoRouteSegmentPrivate *clone() = 0;
 
     bool operator ==(const QGeoRouteSegmentPrivate &other) const;
 
@@ -89,6 +90,9 @@ public:
     virtual void setNextRouteSegment(const QExplicitlySharedDataPointer<QGeoRouteSegmentPrivate> &next);
 
     QExplicitlySharedDataPointer<QGeoRouteSegmentPrivate> m_nextSegment;
+
+protected:
+    virtual bool equals(const QGeoRouteSegmentPrivate &other) const;
 };
 
 
@@ -99,6 +103,7 @@ public:
     QGeoRouteSegmentPrivateDefault();
     QGeoRouteSegmentPrivateDefault(const QGeoRouteSegmentPrivateDefault &other);
     ~QGeoRouteSegmentPrivateDefault();
+    virtual QGeoRouteSegmentPrivate *clone() override;
 
     bool operator ==(const QGeoRouteSegmentPrivateDefault &other) const;
 
diff --git a/tests/auto/declarative_core/tst_routing.qml b/tests/auto/declarative_core/tst_routing.qml
index c090a5ea..b81426a3 100644
--- a/tests/auto/declarative_core/tst_routing.qml
+++ b/tests/auto/declarative_core/tst_routing.qml
@@ -774,6 +774,9 @@ Item {
             compare (model.get(0).path.length, 5)
             compare (model.get(0).path[0].latitude, filledRouteQuery.waypoints[0].latitude)
 
+            if (label === "routeModelAutomaticAltImpl") // Test that it is an altImpl
+                compare(model.get(0).travelTime,  123456)
+
             // Remove a waypoint and check that autoupdate works
             filledRouteQuery.removeWaypoint(fcoordinate2)
             tryCompare (spy, "count", 2)
diff --git a/tests/auto/geotestplugin/qgeoroutingmanagerengine_test.h b/tests/auto/geotestplugin/qgeoroutingmanagerengine_test.h
index c77cb0a5..b7f6030e 100644
--- a/tests/auto/geotestplugin/qgeoroutingmanagerengine_test.h
+++ b/tests/auto/geotestplugin/qgeoroutingmanagerengine_test.h
@@ -50,6 +50,11 @@ public:
     QGeoRoutePrivateDefaultAlt(const QGeoRoutePrivateDefaultAlt &other)
         : QGeoRoutePrivateDefault(other) {}
     ~QGeoRoutePrivateDefaultAlt() {}
+
+    int travelTime() const override
+    {
+        return 123456; // To identify this is actually a QGeoRoutePrivateDefaultAlt
+    }
 };
 
 class QGeoRouteAlt : public QGeoRoute
diff --git a/tests/auto/qgeomaneuver/qgeomaneuver.pro b/tests/auto/qgeomaneuver/qgeomaneuver.pro
index 670d0cd6..fac57c96 100644
--- a/tests/auto/qgeomaneuver/qgeomaneuver.pro
+++ b/tests/auto/qgeomaneuver/qgeomaneuver.pro
@@ -8,4 +8,4 @@ HEADERS += ../utils/qlocationtestutils_p.h \
 SOURCES += tst_qgeomaneuver.cpp \
            ../utils/qlocationtestutils.cpp
 
-QT += location testlib
+QT += location-private testlib
diff --git a/tests/auto/qgeomaneuver/tst_qgeomaneuver.cpp b/tests/auto/qgeomaneuver/tst_qgeomaneuver.cpp
index 25d5bf75..c35b57f6 100644
--- a/tests/auto/qgeomaneuver/tst_qgeomaneuver.cpp
+++ b/tests/auto/qgeomaneuver/tst_qgeomaneuver.cpp
@@ -27,7 +27,30 @@
 ****************************************************************************/
 
 #include "tst_qgeomaneuver.h"
+#include <QtLocation/private/qgeomaneuver_p.h>
 
+class QGeoManeuverPrivateDefaultAlt : public QGeoManeuverPrivateDefault
+{
+public:
+    QGeoManeuverPrivateDefaultAlt() {}
+    QGeoManeuverPrivateDefaultAlt(const QGeoManeuverPrivateDefaultAlt &other)
+        : QGeoManeuverPrivateDefault(other) {}
+    ~QGeoManeuverPrivateDefaultAlt() {}
+
+    QString text() const override
+    {
+        return QStringLiteral("QGeoManeuverPrivateDefaultAlt"); // To identify this is actually a QGeoManeuverPrivateDefaultAlt
+    }
+};
+
+class QGeoManeuverAlt : public QGeoManeuver
+{
+public:
+    QGeoManeuverAlt()
+    : QGeoManeuver(QSharedDataPointer<QGeoManeuverPrivate>(new QGeoManeuverPrivateDefaultAlt()))
+    {
+    }
+};
 
 tst_QGeoManeuver::tst_QGeoManeuver()
 {
@@ -241,9 +264,7 @@ void tst_QGeoManeuver::operators(){
     qgeomaneuvercopy->setTimeToNextInstruction(70);
     qgeomaneuvercopy->setDistanceToNextInstruction(56065.45);
 
-// Not passing
    QVERIFY(!(qgeomaneuver->operator ==(*qgeomaneuvercopy)));
-// Not passing
    QVERIFY(qgeomaneuver->operator !=(*qgeomaneuvercopy));
 
     *qgeomaneuvercopy = qgeomaneuvercopy->operator =(*qgeomaneuver);
@@ -253,5 +274,28 @@ void tst_QGeoManeuver::operators(){
     delete qgeomaneuvercopy;
 }
 
+void tst_QGeoManeuver::alternateImplementation()
+{
+    QGeoManeuver qgeomaneuvercopy = *qgeomaneuver;
+
+    QVERIFY(qgeomaneuvercopy == (*qgeomaneuver));
+
+    qgeomaneuvercopy.setDirection(QGeoManeuver::DirectionForward);
+    qgeomaneuvercopy.setInstructionText("Turn left in 80m");
+    qgeomaneuvercopy.setTimeToNextInstruction(70);
+    qgeomaneuvercopy.setDistanceToNextInstruction(56065.45);
+
+    QVERIFY(qgeomaneuvercopy != (*qgeomaneuver));
+
+    QGeoManeuverAlt mAlt;
+    QGeoManeuver m = mAlt;
+
+    QCOMPARE(m.instructionText(), "QGeoManeuverPrivateDefaultAlt");
+    m = qgeomaneuvercopy;
+    QCOMPARE(m.instructionText(), "Turn left in 80m");
+    m = mAlt;
+    QCOMPARE(m.instructionText(), "QGeoManeuverPrivateDefaultAlt");
+}
+
 
 QTEST_APPLESS_MAIN(tst_QGeoManeuver);
diff --git a/tests/auto/qgeomaneuver/tst_qgeomaneuver.h b/tests/auto/qgeomaneuver/tst_qgeomaneuver.h
index aad28c8c..279ed23c 100644
--- a/tests/auto/qgeomaneuver/tst_qgeomaneuver.h
+++ b/tests/auto/qgeomaneuver/tst_qgeomaneuver.h
@@ -69,6 +69,7 @@ private Q_SLOTS:
     void waypoint_data();
     void isValid();
     void operators();
+    void alternateImplementation();
     //End Unit Test for QGeoRouteManeuver
 
 private:
diff --git a/tests/auto/qgeoroute/qgeoroute.pro b/tests/auto/qgeoroute/qgeoroute.pro
index 87ace5ca..e62adcfa 100644
--- a/tests/auto/qgeoroute/qgeoroute.pro
+++ b/tests/auto/qgeoroute/qgeoroute.pro
@@ -6,4 +6,4 @@ TARGET=tst_qgeoroute
 HEADERS += tst_qgeoroute.h
 SOURCES += tst_qgeoroute.cpp
 
-QT += location testlib
+QT += location-private testlib
diff --git a/tests/auto/qgeoroute/tst_qgeoroute.cpp b/tests/auto/qgeoroute/tst_qgeoroute.cpp
index 6cef9862..709597f8 100644
--- a/tests/auto/qgeoroute/tst_qgeoroute.cpp
+++ b/tests/auto/qgeoroute/tst_qgeoroute.cpp
@@ -27,6 +27,7 @@
 ****************************************************************************/
 
 #include "tst_qgeoroute.h"
+#include "../geotestplugin/qgeoroutingmanagerengine_test.h"
 
 
 tst_QGeoRoute::tst_QGeoRoute()
@@ -71,6 +72,13 @@ void tst_QGeoRoute::copy_constructor()
 {
     QGeoRoute *qgeoroutecopy = new QGeoRoute(*qgeoroute);
     QCOMPARE(*qgeoroute, *qgeoroutecopy);
+
+    // CoW
+    qreal distance = qgeoroute->distance();
+    qgeoroutecopy->setDistance(distance + 10.0);
+
+    QVERIFY(*qgeoroute == *qgeoroutecopy); // QGeoRoute uses a QExplicitlySharedDataPointer. no implicit detach()
+
     delete qgeoroutecopy;
 }
 
@@ -263,7 +271,7 @@ void tst_QGeoRoute::operators()
     QVERIFY(qgeoroute->operator ==(*qgeoroutecopy));
     QVERIFY(!qgeoroute->operator !=(*qgeoroutecopy));
 
-    qgeoroute->setDistance(543.324);
+    qgeoroute->setDistance(543.324); // QExplicitlySharedDataPointer does not detach implicitly.
     qgeoroute->setRouteId("RouteId 111");
     qgeoroute->setTravelMode(QGeoRouteRequest::PedestrianTravel);
     qgeoroute->setTravelTime(10);
@@ -282,6 +290,14 @@ void tst_QGeoRoute::operators()
     QVERIFY(qgeoroute->operator ==(*qgeoroutecopy));
     QVERIFY(!qgeoroute->operator !=(*qgeoroutecopy));
 
+
+    QGeoRouteAlt rAlt;
+    QGeoRoute r;
+    QCOMPARE(rAlt.travelTime(), 123456);
+    QCOMPARE(r.travelTime(), 0);
+    r = rAlt;
+    QCOMPARE(r.travelTime(), 123456);
+
     delete qgeoroutecopy;
 }