Cleanup: unify idiosyncratic singletons

The way we handle singletons in QML, QML insists on allocating the
objects. This leads to a very idiosyncratic way of handling
singletons: The global instance pointer is set in the constructor.

Unify all these by implementing a "SillySingleton" template. All
of the weird singleton-classes can derive from this template and
don't have to bother with reimplementing the instance() function
with all the safety-checks, etc.

This serves firstly as documentation but also improves debugging
as we will now see wanted and unwanted creation and destruction
of these weird singletons.

Signed-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>

core/btdiscovery.cpp

@@ -15,7 +15,6 @@ extern QMap<QString, dc_descriptor_t *> descriptorLookup;
 namespace {
 	QHash<QString, QBluetoothDeviceInfo> btDeviceInfo;
 }
-BTDiscovery *BTDiscovery::m_instance = NULL;
 
 static dc_descriptor_t *getDeviceType(QString btName)
 // central function to convert a BT name to a Subsurface known vendor/model pair
@@ -139,11 +138,6 @@ BTDiscovery::BTDiscovery(QObject*) : m_btValid(false),
 	m_showNonDiveComputers(false),
 	discoveryAgent(nullptr)
 {
-	if (m_instance) {
-		qDebug() << "trying to create an additional BTDiscovery object";
-		return;
-	}
-	m_instance = this;
 #if defined(BT_SUPPORT)
 	QLoggingCategory::setFilterRules(QStringLiteral("qt.bluetooth* = true"));
 	BTDiscoveryReDiscover();
@@ -202,19 +196,11 @@ void BTDiscovery::BTDiscoveryReDiscover()
 
 BTDiscovery::~BTDiscovery()
 {
-	m_instance = NULL;
 #if defined(BT_SUPPORT)
 	delete discoveryAgent;
 #endif
 }
 
-BTDiscovery *BTDiscovery::instance()
-{
-	if (!m_instance)
-		m_instance = new BTDiscovery();
-	return m_instance;
-}
-
 #if defined(BT_SUPPORT)
 extern void addBtUuid(QBluetoothUuid uuid);
 extern QHash<QString, QStringList> productList;

core/btdiscovery.h

@@ -10,6 +10,7 @@
 #include <QBluetoothDeviceDiscoveryAgent>
 #include <QBluetoothUuid>
 #include "core/libdivecomputer.h"
+#include "core/singleton.h"
 
 #if defined(Q_OS_ANDROID)
 #include <QAndroidJniObject>
@@ -23,13 +24,12 @@ QString extractBluetoothAddress(const QString &address);
 QString extractBluetoothNameAddress(const QString &address, QString &name);
 QBluetoothDeviceInfo getBtDeviceInfo(const QString &devaddr);
 
-class BTDiscovery : public QObject {
+class BTDiscovery : public QObject, public SillySingleton<BTDiscovery> {
 	Q_OBJECT
 
 public:
 	BTDiscovery(QObject *parent = NULL);
 	~BTDiscovery();
-	static BTDiscovery *instance();
 
 	struct btPairedDevice {
 		QString address;
@@ -57,7 +57,6 @@ public:
 	void discoverAddress(QString address);
 
 private:
-	static BTDiscovery *m_instance;
 	bool m_btValid;
 	bool m_showNonDiveComputers;
 

core/singleton.h

@@ -0,0 +1,74 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * We use singletons in numerous places. In combination with QML this gives
+ * us a very fundamental problem, because QML likes to allocate objects by
+ * itself. There are known workarounds, but currently we use idiosyncratic
+ * singleton classes, which initialize the global instance pointer in the
+ * constructor. Things get even more complicated if the same singleton is used
+ * on mobile and on desktop. The latter might want to simply use the classical
+ * instance() function without having to initialize the singleton first, as
+ * that would beat the purpose of the instance() method.
+ *
+ * The template defined here, SillySingleton, codifies all this. Simply derive
+ * a class from this template:
+ *	class X : public SillySingleton<X> {
+ *		...
+ *	};
+ * This will generate an instance() method. This will do the right thing for
+ * both methods: explicit construction of the singleton via new or implicit
+ * by calling the instance() method. It will also generate warnings if a
+ * singleton class is generated more than once (i.e. first instance() is called
+ * and _then_ new).
+ *
+ * In the long run we should get rid of all users of this class.
+ */
+#ifndef SINGLETON_H
+#define SINGLETON_H
+
+#include <typeinfo>
+#include <QtGlobal>
+
+// 1) Declaration
+template<typename T>
+class SillySingleton {
+	static T *self;
+protected:
+	SillySingleton();
+	~SillySingleton();
+public:
+	static T *instance();
+};
+
+template<typename T>
+T *SillySingleton<T>::self = nullptr;
+
+// 2) Implementation
+
+template<typename T>
+SillySingleton<T>::SillySingleton()
+{
+	if (self)
+		qWarning("Generating second instance of singleton %s", typeid(T).name());
+	self = static_cast<T *>(this);
+	qDebug("Generated singleton %s", typeid(T).name());
+}
+
+template<typename T>
+SillySingleton<T>::~SillySingleton()
+{
+	if (self == this)
+		self = nullptr;
+	else
+		qWarning("Destroying unknown instance of singleton %s", typeid(T).name());
+	qDebug("Destroyed singleton %s", typeid(T).name());
+}
+
+template<typename T>
+T *SillySingleton<T>::instance()
+{
+	if (!self)
+		new T;
+	return self;
+}
+
+#endif

desktop-widgets/mainwindow.cpp

@@ -109,8 +109,6 @@ extern "C" int updateProgress(const char *text)
 	return progressDialogCanceled;
 }
 
-MainWindow *MainWindow::m_Instance = nullptr;
-
 extern "C" void showErrorFromC(char *buf)
 {
 	QString error(buf);
@@ -128,8 +126,6 @@ MainWindow::MainWindow() : QMainWindow(),
 	survey(nullptr),
 	findMovedImagesDialog(nullptr)
 {
-	Q_ASSERT_X(m_Instance == NULL, "MainWindow", "MainWindow recreated!");
-	m_Instance = this;
 	ui.setupUi(this);
 	read_hashes();
 	Command::init();
@@ -357,7 +353,6 @@ MainWindow::MainWindow() : QMainWindow(),
 MainWindow::~MainWindow()
 {
 	write_hashes();
-	m_Instance = nullptr;
 }
 
 void MainWindow::setupSocialNetworkMenu()
@@ -398,11 +393,6 @@ void MainWindow::setDefaultState()
 		ui.bottomLeft->currentWidget()->setEnabled(false);
 }
 
-MainWindow *MainWindow::instance()
-{
-	return m_Instance;
-}
-
 // This gets called after one or more dives were added, edited or downloaded for a dive computer
 void MainWindow::refreshDisplay(bool doRecreateDiveList)
 {

desktop-widgets/mainwindow.h

@@ -21,6 +21,7 @@
 #include "desktop-widgets/filterwidget2.h"
 #include "core/applicationstate.h"
 #include "core/gpslocation.h"
+#include "core/singleton.h"
 
 #define NUM_RECENT_FILES 4
 
@@ -42,7 +43,7 @@ class LocationInformationWidget;
 typedef std::pair<QByteArray, QVariant> WidgetProperty;
 typedef QVector<WidgetProperty> PropertyList;
 
-class MainWindow : public QMainWindow {
+class MainWindow : public QMainWindow, public SillySingleton<MainWindow> {
 	Q_OBJECT
 public:
 	enum {
@@ -61,7 +62,6 @@ public:
 
 	MainWindow();
 	~MainWindow();
-	static MainWindow *instance();
 	void loadRecentFiles();
 	void updateRecentFiles();
 	void updateRecentFilesMenu();
@@ -193,7 +193,6 @@ private:
 	QString filter_open();
 	QString filter_import();
 	QString filter_import_dive_sites();
-	static MainWindow *m_Instance;
 	QString displayedFilename(QString fullFilename);
 	bool askSaveChanges();
 	bool okToClose(QString message);

mobile-widgets/qmlmanager.cpp

@@ -44,7 +44,6 @@
 #include "core/settings/qPrefUnit.h"
 #include "core/trip.h"
 
-QMLManager *QMLManager::m_instance = NULL;
 bool noCloudToCloud = false;
 
 #define RED_FONT QLatin1Literal("<font color=\"red\">")
@@ -150,7 +149,6 @@ QMLManager::QMLManager() : m_locationServiceEnabled(false),
 	m_showNonDiveComputers(false)
 {
 	LOG_STP("qmlmgr starting");
-	m_instance = this;
 	m_lastDevicePixelRatio = qApp->devicePixelRatio();
 	timer.start();
 	connect(qobject_cast<QApplication *>(QApplication::instance()), &QApplication::applicationStateChanged, this, &QMLManager::applicationStateChanged);
@@ -451,12 +449,6 @@ QMLManager::~QMLManager()
 	if (appLogFileOpen)
 		appLogFile.close();
 #endif
-	m_instance = NULL;
-}
-
-QMLManager *QMLManager::instance()
-{
-	return m_instance;
 }
 
 #define CLOUDURL QString(prefs.cloud_base_url)

mobile-widgets/qmlmanager.h

@@ -13,13 +13,14 @@
 #include "core/btdiscovery.h"
 #include "core/gpslocation.h"
 #include "core/downloadfromdcthread.h"
+#include "core/singleton.h"
 #include "qt-models/divelistmodel.h"
 #include "qt-models/completionmodels.h"
 #include "qt-models/divelocationmodel.h"
 
 #define NOCLOUD_LOCALSTORAGE format_string("%s/cloudstorage/localrepo[master]", system_default_directory())
 
-class QMLManager : public QObject {
+class QMLManager : public QObject, public SillySingleton<QMLManager> {
 	Q_OBJECT
 	Q_PROPERTY(QString logText READ logText WRITE setLogText NOTIFY logTextChanged)
 	Q_PROPERTY(bool locationServiceEnabled MEMBER m_locationServiceEnabled WRITE setLocationServiceEnabled NOTIFY locationServiceEnabledChanged)
@@ -89,7 +90,6 @@ public:
 	Q_INVOKABLE void setGitLocalOnly(const bool &value);
 	Q_INVOKABLE void setFilter(const QString filterText);
 
-	static QMLManager *instance();
 	Q_INVOKABLE void registerError(QString error);
 	QString consumeError();
 
@@ -219,7 +219,6 @@ private:
 	bool m_verboseEnabled;
 	GpsLocation *locationProvider;
 	bool m_loadFromCloud;
-	static QMLManager *m_instance;
 	struct dive *deletedDive;
 	struct dive_trip *deletedTrip;
 	QString m_notificationText;

mobile-widgets/qmlprefs.cpp

@@ -8,30 +8,13 @@
 
 
 /*** Global and constructors ***/
-QMLPrefs *QMLPrefs::m_instance = NULL;
-
 QMLPrefs::QMLPrefs() :
 	m_credentialStatus(qPrefCloudStorage::CS_UNKNOWN),
 	m_oldStatus(qPrefCloudStorage::CS_UNKNOWN),
 	m_showPin(false)
 {
-	// This strange construct is needed because QMLEngine calls new and that
-	// cannot be overwritten
-	if (!m_instance)
-		m_instance = this;
 }
 
-QMLPrefs::~QMLPrefs()
-{
-	m_instance = NULL;
-}
-
-QMLPrefs *QMLPrefs::instance()
-{
-	return m_instance;
-}
-
-
 /*** public functions ***/
 const QString QMLPrefs::cloudPassword() const
 {

mobile-widgets/qmlprefs.h

@@ -5,9 +5,9 @@
 #include <QObject>
 #include "core/settings/qPrefCloudStorage.h"
 #include "core/settings/qPrefDisplay.h"
+#include "core/singleton.h"
 
-
+class QMLPrefs : public QObject, public SillySingleton<QMLPrefs> {
-class QMLPrefs : public QObject {
 	Q_OBJECT
 	Q_PROPERTY(QString cloudPassword
 				MEMBER m_cloudPassword
@@ -35,9 +35,6 @@ class QMLPrefs : public QObject {
 				NOTIFY oldStatusChanged)
 public:
 	QMLPrefs();
-	~QMLPrefs();
-
-	static QMLPrefs *instance();
 
 	const QString cloudPassword() const;
 	void setCloudPassword(const QString &cloudPassword);
@@ -66,7 +63,6 @@ private:
 	QString m_cloudPin;
 	QString m_cloudUserName;
 	qPrefCloudStorage::cloud_status m_credentialStatus;
-	static QMLPrefs *m_instance;
 	qPrefCloudStorage::cloud_status m_oldStatus;
 	bool m_showPin;
 

qt-models/divelistmodel.cpp

@@ -134,11 +134,8 @@ QString DiveListSortModel::tripShortDate(const QString &section)
 	return QStringLiteral("%1\n'%2").arg(firstMonth,firstTime.toString("yy"));
 }
 
-DiveListModel *DiveListModel::m_instance = NULL;
-
 DiveListModel::DiveListModel(QObject *parent) : QAbstractListModel(parent)
 {
-	m_instance = this;
 }
 
 void DiveListModel::insertDive(int i)
@@ -274,11 +271,6 @@ QString DiveListModel::startAddDive()
 	return QString::number(d->id);
 }
 
-DiveListModel *DiveListModel::instance()
-{
-	return m_instance;
-}
-
 struct dive *DiveListModel::getDive(int i)
 {
 	if (i < 0 || i >= dive_table.nr) {

qt-models/divelistmodel.h

@@ -6,6 +6,7 @@
 #include <QSortFilterProxyModel>
 
 #include "core/subsurface-qt/DiveObjectHelper.h"
+#include "core/singleton.h"
 
 class DiveListSortModel : public QSortFilterProxyModel
 {
@@ -28,7 +29,7 @@ private:
 	void updateFilterState();
 };
 
-class DiveListModel : public QAbstractListModel
+class DiveListModel : public QAbstractListModel, public SillySingleton<DiveListModel>
 {
 	Q_OBJECT
 public:
@@ -45,7 +46,6 @@ public:
 		DepthDurationRole,
 	};
 
-	static DiveListModel *instance();
 	DiveListModel(QObject *parent = 0);
 	void addDive(const QList<dive *> &listOfDives);
 	void addAllDives();
@@ -63,8 +63,6 @@ public:
 	QString startAddDive();
 	void resetInternalData();
 	Q_INVOKABLE DiveObjectHelper at(int i);
-private:
-	static DiveListModel *m_instance;
 };
 
 #endif // DIVELISTMODEL_H

qt-models/gpslistmodel.cpp

@@ -3,11 +3,8 @@
 #include "core/qthelper.h"
 #include <QVector>
 
-GpsListModel *GpsListModel::m_instance = NULL;
-
 GpsListModel::GpsListModel(QObject *parent) : QAbstractListModel(parent)
 {
-	m_instance = this;
 }
 
 void GpsListModel::update()
@@ -62,9 +59,3 @@ QHash<int, QByteArray> GpsListModel::roleNames() const
 	roles[GpsLongitudeRole] = "longitude";
 	return roles;
 }
-
-GpsListModel *GpsListModel::instance()
-{
-	return m_instance;
-}
-

qt-models/gpslistmodel.h

@@ -3,10 +3,10 @@
 #define GPSLISTMODEL_H
 
 #include "core/gpslocation.h"
-#include <QObject>
+#include "core/singleton.h"
 #include <QAbstractListModel>
 
-class GpsListModel : public QAbstractListModel
+class GpsListModel : public QAbstractListModel, public SillySingleton<GpsListModel>
 {
 	Q_OBJECT
 public:
@@ -19,7 +19,6 @@ public:
 		GpsWhenRole
 	};
 
-	static GpsListModel *instance();
 	GpsListModel(QObject *parent = 0);
 	void clear();
 	int rowCount(const QModelIndex &parent = QModelIndex()) const;
@@ -28,7 +27,6 @@ public:
 	void update();
 private:
 	QVector<gpsTracker> m_gpsFixes;
-	static GpsListModel *m_instance;
 };
 
 #endif // GPSLISTMODEL_H