Skip to content
Permalink
Browse files

Some memory modernization in QgsGpsDetector

But unfortunately the public API used here is extremely fragile and
either crash prone or leaky -- it needs revisiting for 4.0
  • Loading branch information
nyalldawson committed Jan 7, 2020
1 parent d8a4081 commit 4179b171443522094f82370f9bfdbdf5b9014b59
@@ -32,7 +32,14 @@ Class to detect the GPS port
void connDestroyed( QObject * );

signals:
void detected( QgsGpsConnection * );


void detected( QgsGpsConnection *connection );
%Docstring
Emitted when the GPS connection has been detected. A single connection must listen for this signal and
immediately take ownership of the ``connection`` object.
%End

void detectionFailed();

};
@@ -60,8 +60,6 @@ QList< QPair<QString, QString> > QgsGpsDetector::availablePorts()

QgsGpsDetector::QgsGpsDetector( const QString &portName )
{
mConn = nullptr;

#if defined( HAVE_QT5SERIALPORT )
mBaudList << QSerialPort::Baud4800 << QSerialPort::Baud9600 << QSerialPort::Baud38400 << QSerialPort::Baud57600 << QSerialPort::Baud115200; //add 57600 for SXBlueII GPS unit
#endif
@@ -74,21 +72,13 @@ QgsGpsDetector::QgsGpsDetector( const QString &portName )
{
mPortList << QPair<QString, QString>( portName, portName );
}

mPortIndex = 0;
mBaudIndex = -1;
}

QgsGpsDetector::~QgsGpsDetector()
{
delete mConn;
}
QgsGpsDetector::~QgsGpsDetector() = default;

void QgsGpsDetector::advance()
{
delete mConn;

mConn = nullptr;
mConn.reset();

while ( !mConn )
{
@@ -114,12 +104,12 @@ void QgsGpsDetector::advance()

Q_ASSERT( gpsParams.size() >= 3 );

mConn = new QgsGpsdConnection( gpsParams[0], gpsParams[1].toShort(), gpsParams[2] );
mConn = qgis::make_unique< QgsGpsdConnection >( gpsParams[0], gpsParams[1].toShort(), gpsParams[2] );
}
else if ( mPortList.at( mPortIndex ).first.contains( QLatin1String( "internalGPS" ) ) )
{
#if defined(HAVE_QT_MOBILITY_LOCATION ) || defined(QT_POSITIONING_LIB)
mConn = new QgsQtLocationConnection();
mConn = qgis::make_unique< QgsQtLocationConnection >();
#else
qWarning( "QT_MOBILITY_LOCATION not found and mPortList matches internalGPS, this should never happen" );
#endif
@@ -137,7 +127,7 @@ void QgsGpsDetector::advance()

if ( serial->open( QIODevice::ReadOnly ) )
{
mConn = new QgsNmeaConnection( serial );
mConn = qgis::make_unique< QgsNmeaConnection >( serial );
}
else
{
@@ -149,8 +139,8 @@ void QgsGpsDetector::advance()
}
}

connect( mConn, &QgsGpsConnection::stateChanged, this, static_cast < void ( QgsGpsDetector::* )( const QgsGpsInformation & ) >( &QgsGpsDetector::detected ) );
connect( mConn, &QObject::destroyed, this, &QgsGpsDetector::connDestroyed );
connect( mConn.get(), &QgsGpsConnection::stateChanged, this, static_cast < void ( QgsGpsDetector::* )( const QgsGpsInformation & ) >( &QgsGpsDetector::detected ) );
connect( mConn.get(), &QObject::destroyed, this, &QgsGpsDetector::connDestroyed );

// leave 2s to pickup a valid string
QTimer::singleShot( 2000, this, &QgsGpsDetector::advance );
@@ -167,18 +157,20 @@ void QgsGpsDetector::detected( const QgsGpsInformation &info )
}
else if ( mConn->status() == QgsGpsConnection::GPSDataReceived )
{
// signal detection
QgsGpsConnection *conn = mConn;
mConn = nullptr;
emit detected( conn );
// signal detected

// let's hope there's a single, unique connection to this signal... otherwise... boom
emit detected( mConn.release() );

deleteLater();
}
}

void QgsGpsDetector::connDestroyed( QObject *obj )
{
if ( obj == mConn )
// WTF? This whole class needs re-writing...
if ( obj == mConn.get() )
{
mConn = nullptr;
mConn.release();
}
}
@@ -21,6 +21,7 @@
#include <QObject>
#include <QList>
#include <QPair>
#include <memory>

#include "qgis_core.h"

@@ -46,16 +47,25 @@ class CORE_EXPORT QgsGpsDetector : public QObject
void connDestroyed( QObject * );

signals:
void detected( QgsGpsConnection * );

// TODO QGIS 4.0 - this is horrible, fragile, leaky and crash prone API.
// don't transfer ownership with this signal, and add an explicit takeConnection member!

/**
* Emitted when the GPS connection has been detected. A single connection must listen for this signal and
* immediately take ownership of the \a connection object.
*/
void detected( QgsGpsConnection *connection );

void detectionFailed();

private:
int mPortIndex;
int mBaudIndex;
int mPortIndex = 0;
int mBaudIndex = -1;
QList< QPair< QString, QString > > mPortList;
QList<qint32> mBaudList;

QgsGpsConnection *mConn = nullptr;
std::unique_ptr< QgsGpsConnection > mConn;
};

#endif // QGSGPSDETECTOR_H

0 comments on commit 4179b17

Please sign in to comment.
You can’t perform that action at this time.