[PATCH] Separate download code sharable between desktop & mobile

Dirk Hohndel dirk at hohndel.org
Sun Jan 31 10:36:21 PST 2016


On Sun, Jan 31, 2016 at 07:47:40PM +0200, Willem Ferguson wrote:
>     [PATCH] Separate download code sharable between desktop & mobile
> 
>     This is a very first step that extracts code from
>     downloadfromdivecomputer into a separate class called
>     "downloadmanager". The purpose of this patch is to do
>     some separation of sharable code without affecting the desktop
>     functionality. The code for mobile and its operation is not
>     affected at all.
> 
>     Signed-off-by: Willem Ferguson <willemferguson at zoology.up.ac.za>
> 
> This is a concept that needs review by individuals better with C++ than I am
> (almost everyone on the list).

Not sure that's true, but I'll give it a try. We certainly have people
with a lot more C++ background here.

> I tested this code by downloading dives using my Uwatec dc. Next step is to
> define a model class for transferring dive computer
> names to the QML front end. The models used in the existing widget are
> almost surely not usable for interacting with the QML.

I'm not sure why you say that. The issue that I see is the filtering
that's missing (i.e., just shoing the support DCs), but the models should
work in QML with very little modification.

> diff --git a/desktop-widgets/CMakeLists.txt b/desktop-widgets/CMakeLists.txt
> index dc55035..ec077d7 100644
> --- a/desktop-widgets/CMakeLists.txt
> +++ b/desktop-widgets/CMakeLists.txt
> @@ -57,6 +57,7 @@ set(SUBSURFACE_INTERFACE
>  	diveplanner.cpp
>  	diveshareexportdialog.cpp
>  	downloadfromdivecomputer.cpp
> +	downloadmanager.cpp

That filename is confusing... based on reading what you are doing there
below I don't think this is the "manager".
Also, since it is supposed to be used both from desktop and mobile it
would have to be either in subsurface-core or (with a little restructuring
- as really it should hold only the models) in qt-models.

> diff --git a/desktop-widgets/downloadfromdivecomputer.cpp b/desktop-widgets/downloadfromdivecomputer.cpp
> index 4c8fa6b..20375ac 100644
> --- a/desktop-widgets/downloadfromdivecomputer.cpp
> +++ b/desktop-widgets/downloadfromdivecomputer.cpp
> @@ -5,6 +5,7 @@
>  #include "display.h"
>  #include "uemis.h"
>  #include "models.h"
> +#include "downloadmanager.h"
>  
>  #include <QTimer>
>  #include <QFileDialog>
> @@ -47,6 +48,9 @@ DownloadFromDCWidget::DownloadFromDCWidget(QWidget *parent, Qt::WindowFlags f) :
>  	ostcFirmwareCheck(0),
>  	currentState(INITIAL)
>  {
> +
> +    downloadHelper = new DownloadManager();
> +

Whitespace. You're indenting with four spaces instead of tab. You need to
install the Subsurface settings in QtCreator would be my guess. Look for
details how to do this in the CodingStyle document.

> @@ -64,8 +68,7 @@ DownloadFromDCWidget::DownloadFromDCWidget(QWidget *parent, Qt::WindowFlags f) :
>  
>  	progress_bar_text = "";
>  
> -	fill_computer_list();
> -
> +    downloadHelper->fill_computer_list();

So fill_computer_list() right now populates the vendorList which is part
of the DownloadFromDCWidget... not sure how you can do this in a helper
function that's part of a different class without passing that list around
somehow...

> @@ -76,19 +79,20 @@ 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);
> -	if (default_dive_computer_vendor) {
> +    vendorModel = new QStringListModel(downloadHelper->vendorList);
> +    ui.vendor->setModel(vendorModel);

Ah, that's how you are doing it... I don't think that's a good way to do
this. I'd create the model in the helper and pass that around instead of
just a string list.

> +
> +    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);
> +        productModel = new QStringListModel(downloadHelper->productList[default_dive_computer_vendor]);
> +        ui.product->setModel(productModel);
>  		if (default_dive_computer_product)
>  			ui.product->setCurrentIndex(ui.product->findText(default_dive_computer_product));
>  	}
> -	if (default_dive_computer_device)
> +    if (default_dive_computer_device)

All the white space changes make this diff really hard to read...

> --- /dev/null
> +++ b/desktop-widgets/downloadmanager.cpp
> @@ -0,0 +1,75 @@
> +#include <QStringList>
> +#include <QString>
> +#include <QHash>
> +#include <QMap>
> +#include "libdivecomputer.h"
> +#include "divecomputer.h"
> +#include "downloadmanager.h"
> +
> +DownloadManager::DownloadManager(QObject *parent) :
> +    QObject(parent) {}
> +
> +DownloadManager::~DownloadManager() {}
> +
> +/* ------------------------ 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.

So it shouldn't be in "desktop-widgets".

> + ----------------------------------------------------------------------- */
> +void DownloadManager::fill_computer_list()
> +{
> +    struct mydescriptor {
> +        const char *vendor;
> +        const char *product;
> +        dc_family_t type;
> +        unsigned int model;
> +    };
> +
> +    struct mydescriptor *mydescriptor;
> +
> +    dc_iterator_t *iterator = NULL;
> +    dc_descriptor_t *descriptor = NULL;
> +
> +    dc_descriptor_iterator(&iterator);
> +    while (dc_iterator_next(iterator, &descriptor) == DC_STATUS_SUCCESS) {
> +        const char *vendor = dc_descriptor_get_vendor(descriptor);
> +        const char *product = dc_descriptor_get_product(descriptor);
> +
> +        if (!vendorList.contains(vendor))
> +            vendorList.append(vendor);
> +
> +        if (!productList[vendor].contains(product))
> +            productList[vendor].push_back(product);
> +
> +        descriptorLookup[QString(vendor) + QString(product)] = descriptor;
> +    }
> +    dc_iterator_free(iterator);
> +    Q_FOREACH (QString vendor, vendorList)
> +        qSort(productList[vendor]);
> +
> +    /* and add the Uemis Zurich which we are handling internally
> +       THIS IS A HACK as we magically have a data structure here that
> +       happens to match a data structure that is internal to libdivecomputer;
> +       this WILL BREAK if libdivecomputer changes the dc_descriptor struct...
> +       eventually the UEMIS code needs to move into libdivecomputer, I guess */
> +
> +    mydescriptor = (struct mydescriptor *)malloc(sizeof(struct mydescriptor));
> +    mydescriptor->vendor = "Uemis";
> +    mydescriptor->product = "Zurich";
> +    mydescriptor->type = DC_FAMILY_NULL;
> +    mydescriptor->model = 0;
> +
> +    if (!vendorList.contains("Uemis"))
> +        vendorList.append("Uemis");
> +
> +    if (!productList["Uemis"].contains("Zurich"))
> +        productList["Uemis"].push_back("Zurich");
> +
> +    descriptorLookup["UemisZurich"] = (dc_descriptor_t *)mydescriptor;
> +
> +    qSort(vendorList);
> +
> +}

I see that you took what must have seemed like a reasonable first step in
abstracting this out by moving just the creation of the QStringList into a
helper function. I keep thinking that we should abstract this out at the
QAbstractListModel level...

Create a simple white list in that model helper function where you can
match the known supported dive computers and return an appropriate model.
And then use that model either in the desktop UI or in the QML UI


/D


More information about the subsurface mailing list