[PATCH] Separate downloadfromdivecomputer code into a separate class
Willem Ferguson
willemferguson at zoology.up.ac.za
Thu Feb 11 04:07:26 PST 2016
Subject: [PATCH] [PATCH] Separate downloadfromdivecomputer code to separate
class
This code has been cleaned up.
1) Code from downloadfromdivecomputer.cpp is moved to a new class
downloadmanager.cpp. Encapsulation of diveloadmanager is
maximised. As development proceeds more code will be
moved to downloadmanager.
2) Getter functions (to downloadmanager variables) are used outside
of downloadmanager.
3) Downloadmanager.cpp is placed in the subsurface-core directory.
4) This code focuses purely on the desktop code. Code for the
mobile app needs to be added.
Signed-off-by: Willem Ferguson <willemferguson at zoology.up.ac.za>
Responses to previous comments in-line below.
Kind regards,
willem
---
On 04/02/2016 19:11, Dirk Hohndel wrote:
> 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...
Downloadmanager is now in the subsurface-core directory
>
>> 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
This was implemented. Also in the other cases pointed out.
>> @@ -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.
I use the subsurface style sheet in Qtcreator. I still see that some
text that I am sure I did not touch
has been moved horizontally, causing differences in whitespace compared
to original. The above is one
of these cases.
>
>> 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
Corrected
>> @@ -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.
My glitch. Corrected.
>
>> 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()
> {
>
> }
Implemented everywhere (I hope)
>
> 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.
I cannot find a definition of preferred comment style in Subsurface.
Many styles are used currently. The above comes from my days of writing
Pascal (later Delphi) when I enforced strict rules for commenting
at the headings of functions. I would be happy to change it to any specific
format.
>
>> +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?
It appears diveloadfromdivecomputer is the only class that uses it.
Therefore
why should it have global scope? Also part of my Pascal background. The
structure
now lies at the global level, where it was before.
>
> /D
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-PATCH-Separate-downloadfromdivecomputer-code-to-sepa.patch
Type: text/x-diff
Size: 16365 bytes
Desc: not available
URL: <http://lists.subsurface-divelog.org/pipermail/subsurface/attachments/20160211/58a73077/attachment-0001.patch>
More information about the subsurface
mailing list