Skip to content
Permalink
Browse files

Revert "Streamline singleton behavior"

This reverts commit b4a8547.

The template based approach was not cross-platform friendly. When
cross-compiling for Android it caused a new instance of every singleton per
shared library.

Mostly using the magic statics pattern instead now:
See http://stackoverflow.com/a/11711991/2319028

Important things to remember for crash on exit:

 * QgsNetworkAccessManager needs to die before QApplication
 * QgsMapLayerRegistry needs to be emptied before QgsProviderRegistry goes away

And finally:
DON'T USE SINGLETONS!
They are for "one and only one" and not for "happens to be only one" situations.
  • Loading branch information
m-kuhn committed Apr 7, 2015
1 parent 96560c8 commit c2fb5e19e23f6a7d197a5449d29ea147e5909ec2
@@ -1000,8 +1000,6 @@ QgisApp::~QgisApp()
delete QgsProject::instance();

delete mPythonUtils;

QgsMapLayerStyleGuiUtils::cleanup();
}

void QgisApp::dragEnterEvent( QDragEnterEvent *event )
@@ -25,6 +25,12 @@
#include "qgsmaplayerstylemanager.h"


QgsMapLayerStyleGuiUtils* QgsMapLayerStyleGuiUtils::instance()
{
static QgsMapLayerStyleGuiUtils sInstance;
return &sInstance;
}

QAction* QgsMapLayerStyleGuiUtils::actionAddStyle( QgsMapLayer* layer, QObject* parent )
{
QAction* a = new QAction( tr( "Add" ), parent );
@@ -18,19 +18,18 @@

#include <QObject>

#include "qgssingleton.h"

class QgsMapLayer;

class QAction;
class QMenu;

/** Various GUI utility functions for dealing with map layer's style manager */
class QgsMapLayerStyleGuiUtils : public QObject, public QgsSingleton<QgsMapLayerStyleGuiUtils>
class QgsMapLayerStyleGuiUtils : public QObject
{
Q_OBJECT
public:

public:
static QgsMapLayerStyleGuiUtils* instance();
QAction* actionAddStyle( QgsMapLayer* layer, QObject* parent = 0 );
QAction* actionRemoveStyle( QgsMapLayer* layer, QObject* parent = 0 );
QAction* actionRenameStyle( QgsMapLayer* layer, QObject* parent = 0 );
@@ -548,7 +548,6 @@ SET(QGIS_CORE_HDRS
qgsscalecalculator.h
qgsscaleutils.h
qgssimplifymethod.h
qgssingleton.h
qgssnapper.h
qgssnappingutils.h
qgsspatialindex.h
@@ -60,6 +60,12 @@ QgsPaintEffectRegistry::~QgsPaintEffectRegistry()
mMetadata.clear();
}

QgsPaintEffectRegistry* QgsPaintEffectRegistry::instance()
{
static QgsPaintEffectRegistry sInstance;
return &sInstance;
}

QgsPaintEffectAbstractMetadata *QgsPaintEffectRegistry::effectMetadata( const QString &name ) const
{
if ( mMetadata.contains( name ) )
@@ -16,7 +16,6 @@
#ifndef QGSPAINTEFFECTREGISTRY_H
#define QGSPAINTEFFECTREGISTRY_H

#include "qgssingleton.h"
#include "qgis.h"
#include <QDomElement>
#include <QDomDocument>
@@ -152,9 +151,10 @@ class CORE_EXPORT QgsPaintEffectMetadata : public QgsPaintEffectAbstractMetadata
*
* \note Added in version 2.9
*/
class CORE_EXPORT QgsPaintEffectRegistry : public QgsSingleton<QgsPaintEffectRegistry>
class CORE_EXPORT QgsPaintEffectRegistry
{
public:
static QgsPaintEffectRegistry* instance();

/** Returns the metadata for a specific effect.
* @param name unique string name for paint effect class
@@ -194,8 +194,6 @@ class CORE_EXPORT QgsPaintEffectRegistry : public QgsSingleton<QgsPaintEffectReg
~QgsPaintEffectRegistry();

QMap<QString, QgsPaintEffectAbstractMetadata*> mMetadata;

friend class QgsSingleton<QgsPaintEffectRegistry>; // Let QgsSingleton access private constructor
};

#endif //QGSPAINTEFFECTREGISTRY_H
@@ -623,13 +623,6 @@ void QgsApplication::initQgis()

void QgsApplication::exitQgis()
{
// Cleanup known singletons
QgsMapLayerRegistry::cleanup();
QgsNetworkAccessManager::cleanup();
QgsCoordinateTransformCache::cleanup();
QgsDataItemProviderRegistry::cleanup();

// Cleanup providers
delete QgsProviderRegistry::instance();
}

@@ -18,15 +18,20 @@
#include "qgscrscache.h"
#include "qgscoordinatetransform.h"


QgsCoordinateTransformCache* QgsCoordinateTransformCache::instance()
{
static QgsCoordinateTransformCache mInstance;
return &mInstance;
}

QgsCoordinateTransformCache::~QgsCoordinateTransformCache()
{
QHash< QPair< QString, QString >, QgsCoordinateTransform* >::const_iterator tIt = mTransforms.constBegin();
for ( ; tIt != mTransforms.constEnd(); ++tIt )
{
delete tIt.value();
}

mTransforms.clear();
}

const QgsCoordinateTransform* QgsCoordinateTransformCache::transform( const QString& srcAuthId, const QString& destAuthId, int srcDatumTransform, int destDatumTransform )
@@ -19,16 +19,16 @@
#define QGSCRSCACHE_H

#include "qgscoordinatereferencesystem.h"
#include "qgssingleton.h"
#include <QHash>

class QgsCoordinateTransform;

/**Cache coordinate transform by authid of source/dest transformation to avoid the
overhead of initialisation for each redraw*/
class CORE_EXPORT QgsCoordinateTransformCache : public QgsSingleton<QgsCoordinateTransformCache>
class CORE_EXPORT QgsCoordinateTransformCache
{
public:
static QgsCoordinateTransformCache* instance();
~QgsCoordinateTransformCache();
/**Returns coordinate transformation. Cache keeps ownership
@param srcAuthId auth id string of source crs
@@ -41,6 +41,7 @@ class CORE_EXPORT QgsCoordinateTransformCache : public QgsSingleton<QgsCoordinat
void invalidateCrs( const QString& crsAuthId );

private:
static QgsCoordinateTransformCache* mInstance;
QMultiHash< QPair< QString, QString >, QgsCoordinateTransform* > mTransforms; //same auth_id pairs might have different datum transformations
};

@@ -79,6 +79,12 @@ QgsDataItemProviderRegistry::QgsDataItemProviderRegistry()
}
}

QgsDataItemProviderRegistry* QgsDataItemProviderRegistry::instance()
{
static QgsDataItemProviderRegistry sInstance;
return &sInstance;
}

QgsDataItemProviderRegistry::~QgsDataItemProviderRegistry()
{
qDeleteAll( mProviders );
@@ -16,8 +16,6 @@
#ifndef QGSDATAITEMPROVIDERREGISTRY_H
#define QGSDATAITEMPROVIDERREGISTRY_H

#include "qgssingleton.h"

#include <QList>

class QgsDataItemProvider;
@@ -28,9 +26,11 @@ class QgsDataItemProvider;
*
* @note added in 2.10
*/
class CORE_EXPORT QgsDataItemProviderRegistry : public QgsSingleton<QgsDataItemProviderRegistry>
class CORE_EXPORT QgsDataItemProviderRegistry
{
public:
static QgsDataItemProviderRegistry* instance();

~QgsDataItemProviderRegistry();

//! Get list of available providers
@@ -45,8 +45,6 @@ class CORE_EXPORT QgsDataItemProviderRegistry : public QgsSingleton<QgsDataItemP
private:
QgsDataItemProviderRegistry();

friend class QgsSingleton<QgsDataItemProviderRegistry>; // Let QgsSingleton access private constructor

//! available providers. this class owns the pointers
QList<QgsDataItemProvider*> mProviders;
};
@@ -19,6 +19,15 @@
#include "qgsmaplayer.h"
#include "qgslogger.h"

//
// Static calls to enforce singleton behaviour
//
QgsMapLayerRegistry *QgsMapLayerRegistry::instance()
{
static QgsMapLayerRegistry sInstance;
return &sInstance;
}

//
// Main class begins now...
//
@@ -23,20 +23,21 @@
#include <QSet>
#include <QObject>
#include <QStringList>

#include "qgssingleton.h"
class QString;
class QgsMapLayer;

/** \ingroup core
* This class tracks map layers that are currently loaded and provides
* a means to fetch a pointer to a map layer and delete it.
*/
class CORE_EXPORT QgsMapLayerRegistry : public QObject, public QgsSingleton<QgsMapLayerRegistry>
class CORE_EXPORT QgsMapLayerRegistry : public QObject
{
Q_OBJECT

public:
//! Returns the instance pointer, creating the object on the first call
static QgsMapLayerRegistry * instance();

//! Return the number of registered layers.
int count();

@@ -242,8 +243,6 @@ class CORE_EXPORT QgsMapLayerRegistry : public QObject, public QgsSingleton<QgsM

QMap<QString, QgsMapLayer*> mMapLayers;
QSet<QgsMapLayer*> mOwnedLayers;

friend class QgsSingleton<QgsMapLayerRegistry>; // Let QgsSingleton access private constructor
}; // class QgsMapLayerRegistry

#endif //QgsMapLayerRegistry_H
@@ -86,6 +86,15 @@ class QgsNetworkProxyFactory : public QNetworkProxyFactory
}
};

//
// Static calls to enforce singleton behaviour
//
QgsNetworkAccessManager* QgsNetworkAccessManager::instance()
{
static QgsNetworkAccessManager* sInstance( new QgsNetworkAccessManager( QApplication::instance() ) );
return sInstance;
}

QgsNetworkAccessManager::QgsNetworkAccessManager( QObject *parent )
: QNetworkAccessManager( parent )
, mUseSystemProxy( false )
@@ -24,8 +24,6 @@
#include <QNetworkProxy>
#include <QNetworkRequest>

#include "qgssingleton.h"

/*
* \class QgsNetworkAccessManager
* \brief network access manager for QGIS
@@ -43,11 +41,15 @@
* that the fallback proxy should not be used for, then no proxy will be used.
*
*/
class CORE_EXPORT QgsNetworkAccessManager : public QNetworkAccessManager, public QgsSingleton<QgsNetworkAccessManager>
class CORE_EXPORT QgsNetworkAccessManager : public QNetworkAccessManager
{
Q_OBJECT

public:
//! returns a pointer to the single instance
// and creates that instance on the first call.
static QgsNetworkAccessManager* instance();

QgsNetworkAccessManager( QObject *parent = 0 );

//! destructor
@@ -29,6 +29,7 @@
#include "qgsmessagelog.h"
#include "qgsprovidermetadata.h"
#include "qgsvectorlayer.h"
#include "qgsmaplayerregistry.h"


// typedefs for provider plugin functions of interest
@@ -204,11 +205,13 @@ typedef void cleanupProviderFunction_t();

QgsProviderRegistry::~QgsProviderRegistry()
{
QgsMapLayerRegistry::instance()->removeAllMapLayers();

Providers::const_iterator it = mProviders.begin();

while ( it != mProviders.end() )
{
QgsDebugMsg( QString( "cleanup: %1" ).arg( it->first ) );
QgsDebugMsg( QString( "cleanup:%1" ).arg( it->first ) );
QString lib = it->second->library();
QLibrary myLib( lib );
if ( myLib.isLoaded() )
@@ -405,7 +408,7 @@ QWidget* QgsProviderRegistry::selectWidget( const QString & providerKey,

#if QT_VERSION >= 0x050000
QFunctionPointer QgsProviderRegistry::function( QString const & providerKey,
QString const & functionName )
QString const & functionName )
{
QLibrary myLib( library( providerKey ) );

This file was deleted.

5 comments on commit c2fb5e1

@nyalldawson

This comment has been minimized.

Copy link
Contributor

nyalldawson replied Apr 28, 2015

They are for "one and only one" and not for "happens to be only one" situations.
@m-kuhn so what about registries?Do you consider them a valid use of singletons?

@NathanW2

This comment has been minimized.

Copy link
Member

NathanW2 replied Apr 28, 2015

@nyalldawson

This comment has been minimized.

Copy link
Contributor

nyalldawson replied Apr 28, 2015

@NathanW2 I might be missing something, but wouldn't having non-private constructors on all the singleton classes have a similar effect without the compilation cost of adding them to QgsApplication?

@m-kuhn

This comment has been minimized.

Copy link
Member Author

m-kuhn replied Apr 29, 2015

Before creating a singleton one should think about if something would not rather be placed as child of something. And if there's a good reason to have one and only one object of something.

  • The map layer registry should be in the project as that's the scope of the layers.
  • The project as a singleton is broken by design anyway. One of the best examples of something that was once thought to be unique and is by no means unique.
  • Most of the other registries should probably be handled on application level (and many of them are only used in few places and can be passed there as regular parameters)
  • Singletons may be ok for shared system resources like network access or a postgres connection pool.

Avoiding singletons helps to modularize the application and extending it to multi-project, multi-user, multi-threaded situations.

@jef-n

This comment has been minimized.

Copy link
Member

jef-n replied Apr 29, 2015

The server should also be considered. Currently QgsProject is not used there. Probably because it's singleton and the server can handle more than one project at a time - also there is a layer cache that is not tied to the project as the same layer might appear in multiple projects (eg. embedded).

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