Extract the device_data_t into helper class

Keeping the Desktop and QML versions of Subsurface
using the same codebase will keep the code saner,
this change makes the Desktop version use the
DCDeviceData helper sturct that encapsulates
the device_data_t member for easy access on the
QML. This also helped move a bit of initializations
from the UI to the Core - and that's always good.

Signed-off-by: Tomaz Canabrava <tcanabrava@kde.org>
Signed-off-by: Dirk Hohndel <dirk@hohndel.org>

core/downloadfromdcthread.cpp

@@ -15,28 +15,27 @@ static QString str_error(const char *fmt, ...)
 	return str;
 }
 
-DownloadThread::DownloadThread(QObject *parent, device_data_t *data) : QThread(parent),
+DownloadThread::DownloadThread()
-	data(data)
 {
-	data->download_table = nullptr;
 }
 
 void DownloadThread::setDiveTable(struct dive_table* table)
 {
-	data->download_table = table;
+	m_data.setDiveTable(table);
 }
 
 void DownloadThread::run()
 {
-	Q_ASSERT(data->download_table != nullptr);
+	auto internalData = m_data.internalData();
+	Q_ASSERT(internalData->download_table != nullptr);
 	const char *errorText;
 	import_thread_cancelled = false;
-	if (!strcmp(data->vendor, "Uemis"))
+	if (!strcmp(internalData->vendor, "Uemis"))
-		errorText = do_uemis_import(data);
+		errorText = do_uemis_import(internalData);
 	else
-		errorText = do_libdivecomputer_import(data);
+		errorText = do_libdivecomputer_import(internalData);
 	if (errorText)
-		error = str_error(errorText, data->devname, data->vendor, data->product);
+		error = str_error(errorText, internalData->devname, internalData->vendor, internalData->product);
 }
 
 void fill_computer_list()
@@ -86,6 +85,20 @@ void fill_computer_list()
 	qSort(vendorList);
 }
 
+DCDeviceData::DCDeviceData(QObject *parent) : QObject(parent)
+{
+	memset(&data, 0, sizeof(data));
+	data.trip = nullptr;
+	data.download_table = nullptr;
+	data.diveid = 0;
+	data.deviceid = 0;
+}
+
+DCDeviceData & DownloadThread::data()
+{
+	return m_data;
+}
+
 QString DCDeviceData::vendor() const
 {
 	return data.vendor;
@@ -104,7 +117,6 @@ QString DCDeviceData::devName() const
 QString DCDeviceData::descriptor() const
 {
 	return "";
-//	return data.descriptor;
 }
 
 bool DCDeviceData::bluetoothMode() const
@@ -176,3 +188,33 @@ void DCDeviceData::setDiveId(int diveId)
 {
 	data.diveid = diveId;
 }
+
+void DCDeviceData::setSaveDump(bool save)
+{
+	data.libdc_dump = save;
+}
+
+bool DCDeviceData::saveDump() const
+{
+	return data.libdc_dump;
+}
+
+void DCDeviceData::setSaveLog(bool saveLog)
+{
+	data.libdc_log = saveLog;
+}
+
+bool DCDeviceData::saveLog() const
+{
+	return data.libdc_log;
+}
+
+device_data_t* DCDeviceData::internalData()
+{
+	return &data;
+}
+
+void DCDeviceData::setDiveTable(struct dive_table* downloadTable)
+{
+	data.download_table = downloadTable;
+}

core/downloadfromdcthread.h

@@ -20,6 +20,8 @@ class DCDeviceData : public QObject {
 	Q_PROPERTY(bool createNewTrip READ createNewTrip WRITE setCreateNewTrip)
 	Q_PROPERTY(int deviceId READ deviceId WRITE setDeviceId)
 	Q_PROPERTY(int diveId READ diveId WRITE setDiveId)
+	Q_PROPERTY(bool saveDump READ saveDump WRITE setSaveDump)
+	Q_PROPERTY(bool saveLog READ saveLog WRITE setSaveLog)
 
 public:
 	DCDeviceData(QObject *parent = nullptr);
@@ -31,9 +33,16 @@ public:
 	bool bluetoothMode() const;
 	bool forceDownload() const;
 	bool createNewTrip() const;
+	bool saveDump() const;
+	bool saveLog() const;
 	int deviceId() const;
 	int diveId() const;
 
+	void setDiveTable(struct dive_table* downloadTable);
+
+	/* this needs to be a pointer to make the C-API happy */
+	device_data_t* internalData();
+
 public slots:
 	void setVendor(const QString& vendor);
 	void setProduct(const QString& product);
@@ -44,7 +53,8 @@ public slots:
 	void setCreateNewTrip(bool create);
 	void setDeviceId(int deviceId);
 	void setDiveId(int diveId);
-
+	void setSaveDump(bool dumpMode);
+	void setSaveLog(bool saveLog);
 private:
 	device_data_t data;
 };
@@ -52,28 +62,32 @@ private:
 class DownloadThread : public QThread {
 	Q_OBJECT
 public:
-	DownloadThread(QObject *parent, device_data_t *data);
+	DownloadThread();
 	void setDiveTable(struct dive_table *table);
 	void run() override;
 
+	DCDeviceData& data();
 	QString error;
 
 private:
-	device_data_t *data;
+	DCDeviceData m_data;
 };
 
+//TODO: QList<product> ?
 struct product {
 	const char *product;
 	dc_descriptor_t *descriptor;
 	struct product *next;
 };
 
+//TODO: QList<vendor> ?
 struct vendor {
 	const char *vendor;
 	struct product *productlist;
 	struct vendor *next;
 };
 
+//TODO: C++ify descriptor?
 struct mydescriptor {
 	const char *vendor;
 	const char *product;

desktop-widgets/downloadfromdivecomputer.cpp

@@ -22,7 +22,6 @@ namespace DownloadFromDcGlobal {
 #define DC_TRANSPORT_BLUETOOTH 1024
 
 DownloadFromDCWidget::DownloadFromDCWidget(QWidget *parent, Qt::WindowFlags f) : QDialog(parent, f),
-	thread(0),
 	downloading(false),
 	previousLast(0),
 	vendorModel(0),
@@ -88,7 +87,6 @@ DownloadFromDCWidget::DownloadFromDCWidget(QWidget *parent, Qt::WindowFlags f) :
 		ui.device->setEditText(dc->dc_device());
 
 	updateState(INITIAL);
-	memset(&data, 0, sizeof(data));
 	ui.ok->setEnabled(false);
 	ui.downloadCancelRetryButton->setEnabled(true);
 	ui.downloadCancelRetryButton->setText(tr("Download"));
@@ -188,7 +186,7 @@ void DownloadFromDCWidget::updateState(states state)
 	// got an error
 	else if (state == ERROR) {
 		timer->stop();
-		QMessageBox::critical(this, TITLE_OR_TEXT(tr("Error"), this->thread->error), QMessageBox::Ok);
+		QMessageBox::critical(this, TITLE_OR_TEXT(tr("Error"), thread.error), QMessageBox::Ok);
 		markChildrenAsEnabled();
 		progress_bar_text = "";
 		ui.progressBar->hide();
@@ -263,61 +261,58 @@ void DownloadFromDCWidget::on_downloadCancelRetryButton_clicked()
 	ui.cancel->setEnabled(false);
 	ui.downloadCancelRetryButton->setText(tr("Cancel download"));
 
-	// I don't really think that create/destroy the thread
+	auto& data = thread.data();
-	// is really necessary.
+	data.setVendor(ui.vendor->currentText());
-	if (thread) {
+	data.setProduct(ui.product->currentText());
-		thread->deleteLater();
-	}
-
-	data.vendor = strdup(ui.vendor->currentText().toUtf8().data());
-	data.product = strdup(ui.product->currentText().toUtf8().data());
 #if defined(BT_SUPPORT)
-	data.bluetooth_mode = ui.bluetoothMode->isChecked();
+	data.setBluetoothMode(ui.bluetoothMode->isChecked());
-	if (data.bluetooth_mode && btDeviceSelectionDialog != NULL) {
+	if (data.bluetoothMode() && btDeviceSelectionDialog != NULL) {
 		// Get the selected device address
-		data.devname = strdup(btDeviceSelectionDialog->getSelectedDeviceAddress().toUtf8().data());
+		data.setDevName(btDeviceSelectionDialog->getSelectedDeviceAddress());
 	} else
 		// this breaks an "else if" across lines... not happy...
 #endif
-	if (same_string(data.vendor, "Uemis")) {
+	if (data.vendor() == "Uemis") {
 		char *colon;
 		char *devname = strdup(ui.device->currentText().toUtf8().data());
 
 		if ((colon = strstr(devname, ":\\ (UEMISSDA)")) != NULL) {
 			*(colon + 2) = '\0';
-			fprintf(stderr, "shortened devname to \"%s\"", data.devname);
+			fprintf(stderr, "shortened devname to \"%s\"", devname);
 		}
-		data.devname = devname;
+		data.setDevName(devname);
 	} else {
-		data.devname = strdup(ui.device->currentText().toUtf8().data());
+		data.setDevName(ui.device->currentText());
 	}
-	data.descriptor = descriptorLookup[ui.vendor->currentText() + ui.product->currentText()];
+	//TODO: Add the descriptor function.
-	data.force_download = ui.forceDownload->isChecked();
+	// data.descriptor = descriptorLookup[ui.vendor->currentText() + ui.product->currentText()];
-	data.create_new_trip = ui.createNewTrip->isChecked();
+	data.setForceDownload(ui.forceDownload->isChecked());
-	data.trip = NULL;
+	data.setCreateNewTrip(ui.createNewTrip->isChecked());
-	data.deviceid = data.diveid = 0;
+	data.setSaveLog(ui.chooseLogFile->isChecked());
+	data.setSaveDump(ui.chooseDumpFile->isChecked());
 
 	auto dc = SettingsObjectWrapper::instance()->dive_computer_settings;
-	dc->setVendor(data.vendor);
+	dc->setVendor(data.vendor());
-	dc->setProduct(data.product);
+	dc->setProduct(data.product());
-	dc->setDevice(data.devname);
+	dc->setDevice(data.devName());
+
 #if defined(BT_SUPPORT) && defined(SSRF_CUSTOM_SERIAL)
 	dc->setDownloadMode(ui.bluetoothMode->isChecked() ? DC_TRANSPORT_BLUETOOTH : DC_TRANSPORT_SERIAL);
 #endif
 
-	thread = new DownloadThread(this, &data);
+	connect(&thread, SIGNAL(finished()),
-	connect(thread, SIGNAL(finished()),
 		this, SLOT(onDownloadThreadFinished()), Qt::QueuedConnection);
 
 	//TODO: Don't call mainwindow.
 	MainWindow *w = MainWindow::instance();
-	connect(thread, SIGNAL(finished()), w, SLOT(refreshDisplay()));
+	connect(&thread, SIGNAL(finished()), w, SLOT(refreshDisplay()));
 
 	// before we start, remember where the dive_table ended
 	previousLast = dive_table.nr;
 
-	thread->setDiveTable(&downloadTable);
+	// TODO: the downloadTable should something inside the thrad or should be something from outside?
-	thread->start();
+	thread.setDiveTable(&downloadTable);
+	thread.start();
 
 	// FIXME: We should get the _actual_ device info instead of whatever
 	// the user entered in the dropdown.
@@ -328,8 +323,8 @@ void DownloadFromDCWidget::on_downloadCancelRetryButton_clicked()
 	// We shouldn't do this for memory dumps.
 	if ((product == "OSTC 3" || product == "OSTC 3+" ||
 			product == "OSTC Cr" || product == "OSTC Sport" ||
-			product == "OSTC 4") && !data.libdc_dump)
+			product == "OSTC 4") && ! data.saveDump())
-		ostcFirmwareCheck = new OstcFirmwareCheck(product);
+	ostcFirmwareCheck = new OstcFirmwareCheck(product);
 }
 
 bool DownloadFromDCWidget::preferDownloaded()
@@ -340,7 +335,7 @@ bool DownloadFromDCWidget::preferDownloaded()
 void DownloadFromDCWidget::checkLogFile(int state)
 {
 	ui.chooseLogFile->setEnabled(state == Qt::Checked);
-	data.libdc_log = (state == Qt::Checked);
+	// TODO: Verify the Thread.
 	if (state == Qt::Checked && logFile.isEmpty()) {
 		pickLogFile();
 	}
@@ -362,7 +357,6 @@ void DownloadFromDCWidget::pickLogFile()
 void DownloadFromDCWidget::checkDumpFile(int state)
 {
 	ui.chooseDumpFile->setEnabled(state == Qt::Checked);
-	data.libdc_dump = (state == Qt::Checked);
 	if (state == Qt::Checked) {
 		if (dumpFile.isEmpty())
 			pickDumpFile();
@@ -398,7 +392,7 @@ void DownloadFromDCWidget::reject()
 void DownloadFromDCWidget::onDownloadThreadFinished()
 {
 	if (currentState == DOWNLOADING) {
-		if (thread->error.isEmpty())
+		if (thread.error.isEmpty())
 			updateState(DONE);
 		else
 			updateState(ERROR);
@@ -411,7 +405,6 @@ void DownloadFromDCWidget::onDownloadThreadFinished()
 	if (downloadTable.nr) {
 		diveImportedModel->setImportedDivesIndexes(0, downloadTable.nr - 1);
 	}
-
 }
 
 void DownloadFromDCWidget::on_cancel_clicked()
@@ -458,8 +451,9 @@ void DownloadFromDCWidget::on_ok_clicked()
 		MainWindow::instance()->dive_list()->selectDive(idx, true);
 	}
 
-	if (ostcFirmwareCheck && currentState == DONE)
+	if (ostcFirmwareCheck && currentState == DONE) {
-		ostcFirmwareCheck->checkLatest(this, &data);
+		ostcFirmwareCheck->checkLatest(this, thread.data().internalData());
+	}
 	accept();
 }
 

desktop-widgets/downloadfromdivecomputer.h

@@ -60,10 +60,9 @@ private:
 	void markChildrenAsEnabled();
 
 	Ui::DownloadFromDiveComputer ui;
-	DownloadThread *thread;
+	DownloadThread thread;
 	bool downloading;
 
-	device_data_t data;
 	int previousLast;
 
 	QStringListModel *vendorModel;