From 5edef5e181e0f3cf2e04182d06b3c781717e6f87 Mon Sep 17 00:00:00 2001 From: Paolo Angelelli Date: Tue, 5 Sep 2017 18:37:09 +0200 Subject: 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 --- src/location/maps/qgeomaneuver.cpp | 20 ++++++--- src/location/maps/qgeomaneuver_p.h | 6 ++- src/location/maps/qgeoroute.cpp | 11 +++++ src/location/maps/qgeoroute_p.h | 7 +++- src/location/maps/qgeoroutesegment.cpp | 16 ++++++++ src/location/maps/qgeoroutesegment_p.h | 5 +++ tests/auto/declarative_core/tst_routing.qml | 3 ++ .../geotestplugin/qgeoroutingmanagerengine_test.h | 5 +++ tests/auto/qgeomaneuver/qgeomaneuver.pro | 2 +- tests/auto/qgeomaneuver/tst_qgeomaneuver.cpp | 48 +++++++++++++++++++++- tests/auto/qgeomaneuver/tst_qgeomaneuver.h | 1 + tests/auto/qgeoroute/qgeoroute.pro | 2 +- tests/auto/qgeoroute/tst_qgeoroute.cpp | 18 +++++++- 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::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())) ); } /*! @@ -307,6 +314,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()) @@ -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::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::clone() +{ + return d->clone(); +} + /*! \class QGeoRouteSegment \inmodule QtLocation @@ -249,6 +255,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()) @@ -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 &next); QExplicitlySharedDataPointer 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 +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(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; } -- cgit v1.2.1