[PATCH] Separate downloadfromdivecomputer code into a separate class

Dirk Hohndel dirk at hohndel.org
Thu Feb 4 09:11:46 PST 2016


On Thu, Feb 04, 2016 at 08:37:57AM +0200, Willem Ferguson wrote:
> 
> At the moment, downloadmanager is in the Desktop-widgets. This is
> for development purposes only because it is handy to have
> downloadfromdivecomputer.cpp close to downloadmanager.cpp

I don't know what that means. "have [it] close to"... you mean in the file
system? That makes no sense to me...

> diff --git a/desktop-widgets/downloadfromdivecomputer.cpp b/desktop-widgets/downloadfromdivecomputer.cpp
> index 4c8fa6b..78a7714 100644
> --- a/desktop-widgets/downloadfromdivecomputer.cpp
> +++ b/desktop-widgets/downloadfromdivecomputer.cpp
> @@ -76,12 +77,10 @@ DownloadFromDCWidget::DownloadFromDCWidget(QWidget *parent, Qt::WindowFlags f) :
>  	ui.unselectAllButton->setEnabled(false);
>  	connect(ui.selectAllButton, SIGNAL(clicked()), diveImportedModel, SLOT(selectAll()));
>  	connect(ui.unselectAllButton, SIGNAL(clicked()), diveImportedModel, SLOT(selectNone()));
> -	vendorModel = new QStringListModel(vendorList);
> -	ui.vendor->setModel(vendorModel);
> +	ui.vendor->setModel(downloadHelper->vendorModel);

Usually the preferred way of doing things like this is to have a "getter"
function instead of exposing the variable as a public. This may seem silly
but there are a number of good reasons to do so (starting with "this
separates the API of your object from the internal implementation" and
"you can handle validation and filtering in that getter function").

So please use DownloadHelper::vendorModel() and turn the vendorModel
member into a private m_vendorModel

>  	if (default_dive_computer_vendor) {
>  		ui.vendor->setCurrentIndex(ui.vendor->findText(default_dive_computer_vendor));
> -		productModel = new QStringListModel(productList[default_dive_computer_vendor]);
> -		ui.product->setModel(productModel);
> +		ui.product->setModel(downloadHelper->productModel);

Same here

> @@ -199,8 +198,8 @@ void DownloadFromDCWidget::on_vendor_currentIndexChanged(const QString &vendor)
>  	if (!currentModel)
>  		return;
>  
> -	productModel = new QStringListModel(productList[vendor]);
> -	ui.product->setModel(productModel);
> +	downloadHelper->productModel->setStringList(downloadHelper->productList[vendor]);

And here.

> @@ -214,7 +213,7 @@ void DownloadFromDCWidget::on_product_currentIndexChanged(const QString &product
>  {
>  	// Set up the DC descriptor
>  	dc_descriptor_t *descriptor = NULL;
> -	descriptor = descriptorLookup[ui.vendor->currentText() + product];
> +	descriptor = downloadHelper->descriptorLookup[ui.vendor->currentText() + product];

And here.

> @@ -380,7 +332,7 @@ void DownloadFromDCWidget::pickLogFile()
>  	QFileInfo fi(filename);
>  	filename = fi.absolutePath().append(QDir::separator()).append("subsurface.log");
>  	logFile = QFileDialog::getSaveFileName(this, tr("Choose file for divecomputer download logfile"),
> -					       filename, tr("Log files (*.log)"));
> +						   filename, tr("Log files (*.log)"));

Another small whitespace thing. You have this a few times. Continuation
lines should be indented with tab and space to align. This should be set
up correctly in QtCreator if you use the settings in CodingStyle. If this
isn't working correctly for you, please let us know.

> diff --git a/desktop-widgets/downloadfromdivecomputer.h b/desktop-widgets/downloadfromdivecomputer.h
> index 7acd49e..15e90f2 100644
> --- a/desktop-widgets/downloadfromdivecomputer.h
> +++ b/desktop-widgets/downloadfromdivecomputer.h
> @@ -10,11 +10,13 @@
>  #include "libdivecomputer.h"
>  #include "configuredivecomputerdialog.h"
>  #include "ui_downloadfromdivecomputer.h"
> +#include "downloadmanager.h"
>  
>  #if defined(BT_SUPPORT)
>  #include "btdeviceselectiondialog.h"
>  #endif
>  
> +

Adding empty lines like this isn't harmful, but it's usually a sign that
you didn't carefully read the patch before sending. :-)

In general we rarely ever have two consecutive empty lines

> @@ -96,15 +98,15 @@ private:
>  	DownloadThread *thread;
>  	bool downloading;
>  
> -	QStringList vendorList;
> -	QHash<QString, QStringList> productList;
> -	QMap<QString, dc_descriptor_t *> descriptorLookup;
> +//	QStringList vendorList;
> +//	QHash<QString, QStringList> productList;
> +//	QMap<QString, dc_descriptor_t *> descriptorLookup;
>  	device_data_t data;
>  	int previousLast;
>  
> -	QStringListModel *vendorModel;
> -	QStringListModel *productModel;
> -	void fill_computer_list();
> +//	QStringListModel *vendorModel;
> +//	QStringListModel *productModel;
> +//	void fill_computer_list();
>  	void fill_device_list(int dc_type);
>  	QString logFile;
>  	QString dumpFile;

That's ok while you work on the code, but once you submit it, just remove
those members, don't comment them out.

> diff --git a/desktop-widgets/downloadmanager.cpp b/desktop-widgets/downloadmanager.cpp
> new file mode 100644
> index 0000000..1100014
> --- /dev/null
> +++ b/desktop-widgets/downloadmanager.cpp
> @@ -0,0 +1,89 @@
> +#include <QStringListModel>
> +#include <QStringList>
> +#include <QString>
> +#include <QHash>
> +#include <QMap>
> +#include "downloadmanager.h"
> +#include "libdivecomputer.h"
> +#include "divecomputer.h"
> +#include "display.h"
> +
> +DownloadManager::DownloadManager(QObject *parent) : QObject(parent) {
> +}

That's a function, so opening '{' at the beginning of the new line

> +DownloadManager::~DownloadManager() {}

Normally I prefer
DownloadManager::~DownloadManager()
{

}

Yes, that wastes vertical space but it's easier to find functions that
way. I know we have code that breaks this... still.

> +
> +/* ------------------------ Fill_computer_list --------------------------
> +1) Fill the QstringList structures containing the vendor names as well as the
> +   products of all the supported dive computers.
> +2) Set up the hash table that points to the descriptor structure for each
> +   dive computer.
> +This method is used by both the desktop and the mobile software.
> + ----------------------------------------------------------------------- */

That comment style is very different from anything we use elsewhere in the
code. Not saying it's bad, just that it's inconsistent.

> +void DownloadManager::fill_computer_list()
> +{
> +	struct mydescriptor {
> +		const char *vendor;
> +		const char *product;
> +		dc_family_t type;
> +		unsigned int model;
> +	};

Why is this a private structure inside the function?


/D


More information about the subsurface mailing list