mirror of
https://github.com/qgis/QGIS.git
synced 2025-12-15 00:07:25 -05:00
Rework QgsGpsDetector to make it memory safe
This is messy, as there's no way we can possibly make the current, stable API of this class safe. We have to resort to an opt-in "safe" mode which exposes a non-dangerous API. This should hopefully fix issues where the qt event loop causes destruction of the detected connection before listener slot is called and is able to take ownership of the signal argument...
This commit is contained in:
Nyall Dawson
parent
a2836644fc
commit
94697e8176
@ -23,11 +23,41 @@ Class to detect the GPS port
|
||||
#include "qgsgpsdetector.h"
|
||||
%End
|
||||
public:
|
||||
QgsGpsDetector( const QString &portName );
|
||||
|
||||
|
||||
QgsGpsDetector( const QString &portName = QString(), bool useUnsafeSignals = true );
|
||||
%Docstring
|
||||
Constructor for QgsGpsDetector.
|
||||
|
||||
If ``portName`` is specified, then only devices from the given port will be scanned. Otherwise
|
||||
all connection types will be attempted (including internal GPS devices).
|
||||
|
||||
Since QGIS 3.38, the ``useUnsafeSignals`` parameter can be set to ``False`` to avoid emitting the
|
||||
dangerous and fragile :py:func:`~QgsGpsDetector.detected` signal. This is highly recommended, but is opt-in to avoid
|
||||
breaking stable QGIS 3.x API. If ``useUnsafeSignals`` is set to ``False``, only the safe :py:func:`~QgsGpsDetector.connectionDetected` signal
|
||||
will be emitted and clients must manually take ownership of the detected connection via a call
|
||||
to :py:func:`~QgsGpsDetector.takeConnection`.
|
||||
%End
|
||||
|
||||
|
||||
~QgsGpsDetector();
|
||||
|
||||
QgsGpsConnection *takeConnection() /TransferBack/;
|
||||
%Docstring
|
||||
Returns the detected GPS connection, and removes it from the detector.
|
||||
|
||||
The caller takes ownership of the connection. Only the first call to this
|
||||
method following a :py:func:`~QgsGpsDetector.connectionDetected` signal will be able to retrieve the
|
||||
detected connection -- subsequent calls will return ``None``.
|
||||
|
||||
.. warning::
|
||||
|
||||
Do not call this method if the useUnsafeSignals option in the
|
||||
QgsGpsDetector constructor was set to ``True``.
|
||||
|
||||
.. versionadded:: 3.38
|
||||
%End
|
||||
|
||||
static QList< QPair<QString, QString> > availablePorts();
|
||||
|
||||
public slots:
|
||||
@ -37,14 +67,28 @@ Class to detect the GPS port
|
||||
|
||||
signals:
|
||||
|
||||
void connectionDetected();
|
||||
%Docstring
|
||||
Emitted when a GPS connection is successfully detected.
|
||||
|
||||
void detected( QgsGpsConnection *connection );
|
||||
Call :py:func:`~QgsGpsDetector.takeConnection` to take ownership of the detected connection.
|
||||
|
||||
.. versionadded:: 3.38
|
||||
%End
|
||||
|
||||
void detected( QgsGpsConnection *connection ) /Deprecated/;
|
||||
%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.
|
||||
|
||||
.. deprecated::
|
||||
This signal is dangerous and extremely unsafe! It is recommended to instead set the ``useUnsafeSignals`` parameter to ``False`` in the QgsGpsDetector constructor and use the safe :py:func:`~QgsGpsDetector.connectionDetected` signal instead.
|
||||
%End
|
||||
|
||||
void detectionFailed();
|
||||
%Docstring
|
||||
Emitted when the detector could not find a valid GPS connection.
|
||||
%End
|
||||
|
||||
};
|
||||
|
||||
|
||||
@ -23,11 +23,41 @@ Class to detect the GPS port
|
||||
#include "qgsgpsdetector.h"
|
||||
%End
|
||||
public:
|
||||
QgsGpsDetector( const QString &portName );
|
||||
|
||||
|
||||
QgsGpsDetector( const QString &portName = QString(), bool useUnsafeSignals = true );
|
||||
%Docstring
|
||||
Constructor for QgsGpsDetector.
|
||||
|
||||
If ``portName`` is specified, then only devices from the given port will be scanned. Otherwise
|
||||
all connection types will be attempted (including internal GPS devices).
|
||||
|
||||
Since QGIS 3.38, the ``useUnsafeSignals`` parameter can be set to ``False`` to avoid emitting the
|
||||
dangerous and fragile :py:func:`~QgsGpsDetector.detected` signal. This is highly recommended, but is opt-in to avoid
|
||||
breaking stable QGIS 3.x API. If ``useUnsafeSignals`` is set to ``False``, only the safe :py:func:`~QgsGpsDetector.connectionDetected` signal
|
||||
will be emitted and clients must manually take ownership of the detected connection via a call
|
||||
to :py:func:`~QgsGpsDetector.takeConnection`.
|
||||
%End
|
||||
|
||||
|
||||
~QgsGpsDetector();
|
||||
|
||||
QgsGpsConnection *takeConnection() /TransferBack/;
|
||||
%Docstring
|
||||
Returns the detected GPS connection, and removes it from the detector.
|
||||
|
||||
The caller takes ownership of the connection. Only the first call to this
|
||||
method following a :py:func:`~QgsGpsDetector.connectionDetected` signal will be able to retrieve the
|
||||
detected connection -- subsequent calls will return ``None``.
|
||||
|
||||
.. warning::
|
||||
|
||||
Do not call this method if the useUnsafeSignals option in the
|
||||
QgsGpsDetector constructor was set to ``True``.
|
||||
|
||||
.. versionadded:: 3.38
|
||||
%End
|
||||
|
||||
static QList< QPair<QString, QString> > availablePorts();
|
||||
|
||||
public slots:
|
||||
@ -37,14 +67,28 @@ Class to detect the GPS port
|
||||
|
||||
signals:
|
||||
|
||||
void connectionDetected();
|
||||
%Docstring
|
||||
Emitted when a GPS connection is successfully detected.
|
||||
|
||||
void detected( QgsGpsConnection *connection );
|
||||
Call :py:func:`~QgsGpsDetector.takeConnection` to take ownership of the detected connection.
|
||||
|
||||
.. versionadded:: 3.38
|
||||
%End
|
||||
|
||||
void detected( QgsGpsConnection *connection ) /Deprecated/;
|
||||
%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.
|
||||
|
||||
.. deprecated::
|
||||
This signal is dangerous and extremely unsafe! It is recommended to instead set the ``useUnsafeSignals`` parameter to ``False`` in the QgsGpsDetector constructor and use the safe :py:func:`~QgsGpsDetector.connectionDetected` signal instead.
|
||||
%End
|
||||
|
||||
void detectionFailed();
|
||||
%Docstring
|
||||
Emitted when the detector could not find a valid GPS connection.
|
||||
%End
|
||||
|
||||
};
|
||||
|
||||
|
||||
@ -57,7 +57,7 @@ void QgsAppGpsConnection::setConnection( QgsGpsConnection *connection )
|
||||
disconnectGps();
|
||||
}
|
||||
|
||||
onConnected( connection );
|
||||
setConnectionPrivate( connection );
|
||||
}
|
||||
|
||||
QgsPoint QgsAppGpsConnection::lastValidLocation() const
|
||||
@ -156,10 +156,11 @@ void QgsAppGpsConnection::connectGps()
|
||||
|
||||
QgsMessageLog::logMessage( QObject::tr( "Firing up GPS detector" ), QObject::tr( "GPS" ), Qgis::Info );
|
||||
|
||||
QgsGpsDetector *detector = new QgsGpsDetector( port );
|
||||
connect( detector, static_cast < void ( QgsGpsDetector::* )( QgsGpsConnection * ) > ( &QgsGpsDetector::detected ), this, &QgsAppGpsConnection::onConnected );
|
||||
connect( detector, &QgsGpsDetector::detectionFailed, this, &QgsAppGpsConnection::onTimeOut );
|
||||
detector->advance(); // start the detection process
|
||||
// note -- QgsGpsDetector internally uses deleteLater to clean itself up!
|
||||
mDetector = new QgsGpsDetector( port, false );
|
||||
connect( mDetector, &QgsGpsDetector::connectionDetected, this, &QgsAppGpsConnection::onConnectionDetected );
|
||||
connect( mDetector, &QgsGpsDetector::detectionFailed, this, &QgsAppGpsConnection::onTimeOut );
|
||||
mDetector->advance(); // start the detection process
|
||||
}
|
||||
|
||||
void QgsAppGpsConnection::disconnectGps()
|
||||
@ -181,6 +182,9 @@ void QgsAppGpsConnection::disconnectGps()
|
||||
|
||||
void QgsAppGpsConnection::onTimeOut()
|
||||
{
|
||||
if ( sender() != mDetector )
|
||||
return;
|
||||
|
||||
QgsMessageLog::logMessage( QObject::tr( "GPS detector reported timeout" ), QObject::tr( "GPS" ), Qgis::Critical );
|
||||
disconnectGps();
|
||||
emit connectionTimedOut();
|
||||
@ -189,11 +193,18 @@ void QgsAppGpsConnection::onTimeOut()
|
||||
showGpsConnectFailureWarning( tr( "TIMEOUT - Failed to connect to GPS device." ) );
|
||||
}
|
||||
|
||||
void QgsAppGpsConnection::onConnected( QgsGpsConnection *conn )
|
||||
void QgsAppGpsConnection::onConnectionDetected()
|
||||
{
|
||||
QgsMessageLog::logMessage( QObject::tr( "GPS detector GOT a connection" ), QObject::tr( "GPS" ), Qgis::Info );
|
||||
if ( sender() != mDetector )
|
||||
return;
|
||||
|
||||
mConnection = conn;
|
||||
QgsMessageLog::logMessage( QObject::tr( "GPS detector GOT a connection" ), QObject::tr( "GPS" ), Qgis::Info );
|
||||
setConnectionPrivate( mDetector->takeConnection() );
|
||||
}
|
||||
|
||||
void QgsAppGpsConnection::setConnectionPrivate( QgsGpsConnection *connection )
|
||||
{
|
||||
mConnection = connection;
|
||||
connect( mConnection, &QgsGpsConnection::stateChanged, this, &QgsAppGpsConnection::stateChanged );
|
||||
connect( mConnection, &QgsGpsConnection::nmeaSentenceReceived, this, &QgsAppGpsConnection::nmeaSentenceReceived );
|
||||
connect( mConnection, &QgsGpsConnection::fixStatusChanged, this, &QgsAppGpsConnection::fixStatusChanged );
|
||||
|
||||
@ -25,6 +25,7 @@ class QgsGpsConnection;
|
||||
class QgsGpsInformation;
|
||||
class QgsPoint;
|
||||
class QgsMessageBarItem;
|
||||
class QgsGpsDetector;
|
||||
|
||||
/**
|
||||
* Manages a single "canonical" GPS connection for use in the QGIS app, eg for displaying GPS
|
||||
@ -140,8 +141,9 @@ class APP_EXPORT QgsAppGpsConnection : public QObject
|
||||
private slots:
|
||||
|
||||
void onTimeOut();
|
||||
void onConnectionDetected();
|
||||
|
||||
void onConnected( QgsGpsConnection *conn );
|
||||
void setConnectionPrivate( QgsGpsConnection *connection );
|
||||
|
||||
private:
|
||||
|
||||
@ -150,6 +152,7 @@ class APP_EXPORT QgsAppGpsConnection : public QObject
|
||||
void showGpsConnectFailureWarning( const QString &message );
|
||||
void showMessage( Qgis::MessageLevel level, const QString &message );
|
||||
|
||||
QPointer< QgsGpsDetector > mDetector;
|
||||
QgsGpsConnection *mConnection = nullptr;
|
||||
QPointer< QgsMessageBarItem > mConnectionMessageItem;
|
||||
};
|
||||
|
||||
@ -64,7 +64,8 @@ QList< QPair<QString, QString> > QgsGpsDetector::availablePorts()
|
||||
return devs;
|
||||
}
|
||||
|
||||
QgsGpsDetector::QgsGpsDetector( const QString &portName )
|
||||
QgsGpsDetector::QgsGpsDetector( const QString &portName, bool useUnsafeSignals )
|
||||
: mUseUnsafeSignals( useUnsafeSignals )
|
||||
{
|
||||
#if defined( HAVE_QTSERIALPORT )
|
||||
mBaudList << QSerialPort::Baud4800 << QSerialPort::Baud9600 << QSerialPort::Baud38400 << QSerialPort::Baud57600 << QSerialPort::Baud115200; //add 57600 for SXBlueII GPS unit
|
||||
@ -87,11 +88,21 @@ QgsGpsDetector::~QgsGpsDetector()
|
||||
QgsMessageLog::logMessage( QObject::tr( "Destroying GPS detector" ), QObject::tr( "GPS" ), Qgis::Info );
|
||||
}
|
||||
|
||||
QgsGpsConnection *QgsGpsDetector::takeConnection()
|
||||
{
|
||||
if ( mUseUnsafeSignals )
|
||||
{
|
||||
QgsDebugError( QStringLiteral( "QgsGpsDetector::takeConnection() incorrectly called when useUnsafeSignals option is in effect" ) );
|
||||
return nullptr;
|
||||
}
|
||||
|
||||
return mConn.release();
|
||||
}
|
||||
|
||||
void QgsGpsDetector::advance()
|
||||
{
|
||||
if ( mConn )
|
||||
{
|
||||
disconnect( mConn.get(), &QObject::destroyed, this, &QgsGpsDetector::connDestroyed );
|
||||
mConn.reset();
|
||||
}
|
||||
|
||||
@ -170,22 +181,22 @@ void QgsGpsDetector::advance()
|
||||
QgsMessageLog::logMessage( QObject::tr( "Have a connection, now listening for messages" ), QObject::tr( "GPS" ), Qgis::Info );
|
||||
|
||||
connect( mConn.get(), &QgsGpsConnection::stateChanged, this, static_cast < void ( QgsGpsDetector::* )( const QgsGpsInformation & ) >( &QgsGpsDetector::detected ) );
|
||||
connect( mConn.get(), &QObject::destroyed, this, &QgsGpsDetector::connDestroyed );
|
||||
if ( mUseUnsafeSignals )
|
||||
{
|
||||
connect( mConn.get(), &QObject::destroyed, this, &QgsGpsDetector::connDestroyed );
|
||||
}
|
||||
|
||||
// leave 2s to pickup a valid string
|
||||
QTimer::singleShot( 2000, this, &QgsGpsDetector::connectionTimeout );
|
||||
}
|
||||
|
||||
void QgsGpsDetector::detected( const QgsGpsInformation &info )
|
||||
void QgsGpsDetector::detected( const QgsGpsInformation & )
|
||||
{
|
||||
Q_UNUSED( info )
|
||||
|
||||
QgsMessageLog::logMessage( QObject::tr( "Detected information" ), QObject::tr( "GPS" ), Qgis::Info );
|
||||
|
||||
if ( !mConn )
|
||||
{
|
||||
// advance if connection was destroyed
|
||||
|
||||
QgsMessageLog::logMessage( QObject::tr( "Got information, but CONNECTION WAS DESTROYED EXTERNALLY!" ), QObject::tr( "GPS" ), Qgis::Critical );
|
||||
advance();
|
||||
}
|
||||
@ -194,8 +205,17 @@ void QgsGpsDetector::detected( const QgsGpsInformation &info )
|
||||
// signal detected
|
||||
QgsMessageLog::logMessage( QObject::tr( "Connection status IS GPSDataReceived" ), QObject::tr( "GPS" ), Qgis::Info );
|
||||
|
||||
// let's hope there's a single, unique connection to this signal... otherwise... boom
|
||||
emit detected( mConn.release() );
|
||||
if ( mUseUnsafeSignals )
|
||||
{
|
||||
// let's hope there's a single, unique connection to this signal... otherwise... boom!
|
||||
Q_NOWARN_DEPRECATED_PUSH
|
||||
emit detected( mConn.release() );
|
||||
Q_NOWARN_DEPRECATED_POP
|
||||
}
|
||||
else
|
||||
{
|
||||
emit connectionDetected();
|
||||
}
|
||||
|
||||
deleteLater();
|
||||
}
|
||||
|
||||
@ -47,7 +47,22 @@ class CORE_EXPORT QgsGpsDetector : public QObject
|
||||
{
|
||||
Q_OBJECT
|
||||
public:
|
||||
QgsGpsDetector( const QString &portName );
|
||||
|
||||
// TODO QGIS 4.0 -- remove useUnsafeSignals option
|
||||
|
||||
/**
|
||||
* Constructor for QgsGpsDetector.
|
||||
*
|
||||
* If \a portName is specified, then only devices from the given port will be scanned. Otherwise
|
||||
* all connection types will be attempted (including internal GPS devices).
|
||||
*
|
||||
* Since QGIS 3.38, the \a useUnsafeSignals parameter can be set to FALSE to avoid emitting the
|
||||
* dangerous and fragile detected() signal. This is highly recommended, but is opt-in to avoid
|
||||
* breaking stable QGIS 3.x API. If \a useUnsafeSignals is set to FALSE, only the safe connectionDetected() signal
|
||||
* will be emitted and clients must manually take ownership of the detected connection via a call
|
||||
* to takeConnection().
|
||||
*/
|
||||
QgsGpsDetector( const QString &portName = QString(), bool useUnsafeSignals = true );
|
||||
|
||||
#if defined( HAVE_QTSERIALPORT )
|
||||
static const QgsSettingsEntryEnumFlag<QSerialPort::StopBits> *settingsGpsStopBits SIP_SKIP;
|
||||
@ -58,6 +73,20 @@ class CORE_EXPORT QgsGpsDetector : public QObject
|
||||
|
||||
~QgsGpsDetector() override;
|
||||
|
||||
/**
|
||||
* Returns the detected GPS connection, and removes it from the detector.
|
||||
*
|
||||
* The caller takes ownership of the connection. Only the first call to this
|
||||
* method following a connectionDetected() signal will be able to retrieve the
|
||||
* detected connection -- subsequent calls will return NULLPTR.
|
||||
*
|
||||
* \warning Do not call this method if the useUnsafeSignals option in the
|
||||
* QgsGpsDetector constructor was set to TRUE.
|
||||
*
|
||||
* \since QGIS 3.38
|
||||
*/
|
||||
QgsGpsConnection *takeConnection() SIP_TRANSFERBACK;
|
||||
|
||||
static QList< QPair<QString, QString> > availablePorts();
|
||||
|
||||
public slots:
|
||||
@ -67,15 +96,26 @@ class CORE_EXPORT QgsGpsDetector : public QObject
|
||||
|
||||
signals:
|
||||
|
||||
// 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 a GPS connection is successfully detected.
|
||||
*
|
||||
* Call takeConnection() to take ownership of the detected connection.
|
||||
*
|
||||
* \since QGIS 3.38
|
||||
*/
|
||||
void connectionDetected();
|
||||
|
||||
/**
|
||||
* 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.
|
||||
*
|
||||
* \deprecated This signal is dangerous and extremely unsafe! It is recommended to instead set the \a useUnsafeSignals parameter to FALSE in the QgsGpsDetector constructor and use the safe connectionDetected() signal instead.
|
||||
*/
|
||||
void detected( QgsGpsConnection *connection );
|
||||
Q_DECL_DEPRECATED void detected( QgsGpsConnection *connection ) SIP_DEPRECATED;
|
||||
|
||||
/**
|
||||
* Emitted when the detector could not find a valid GPS connection.
|
||||
*/
|
||||
void detectionFailed();
|
||||
|
||||
private slots:
|
||||
@ -83,6 +123,7 @@ class CORE_EXPORT QgsGpsDetector : public QObject
|
||||
void connectionTimeout();
|
||||
|
||||
private:
|
||||
bool mUseUnsafeSignals = true;
|
||||
int mPortIndex = 0;
|
||||
int mBaudIndex = -1;
|
||||
QList< QPair< QString, QString > > mPortList;
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user