DiveShare patch

Thiago Macieira thiago at macieira.org
Mon Oct 13 04:03:25 PDT 2014


Hi Salvo

Thanks for the patch. The idea looks good and the networking part per se looks 
correct (even if I haven't run the code), but like Dirk said, it needs a 
couple of fixes before it goes in. Here are a couple of suggestions from me:

On Sunday 12 October 2014 19:41:41 Salvo Tomaselli wrote:
> +DiveShareExportDialog::DiveShareExportDialog(QWidget *parent) :
> +       QDialog(parent),
> +       ui(new Ui::DiveShareExportDialog)
> +{
> +       ui->setupUi(this);
> +       this->nam = new QNetworkAccessManager(this);

Please don't create your own QNetworkAccessManager. We have a global one in 
WebServices::manager(). Just reuse that.

> +       QObject::connect(
> +               this->nam, SIGNAL(finished(QNetworkReply*)),
> +               this, SLOT(finishedSlot(QNetworkReply*))
> +       );
> +}

Which in turn means your slot would be called for all the replies Subsurface 
is getting. It would be better if you connected to each QNetworkReply object 
you get instead of this global one.

> +void DiveShareExportDialog::getUID() {
> +       QDesktopServices::openUrl(QUrl(DIVESHARE_BASE_URI "/secret"));
> +}

Nitpick: here and in a couple of other functions, you have inconsistent style. 
The opening { for functions and global structs should go to the next line.

I'd also recommend renaming this function. Even though "get" here is an action 
being taken, everyone reading the code will think of a getter function.

> +QString generate_html_list(QByteArray data) {

This function should be static.

> +       if (reply->error() != 0) {
> +               const char* error;
> +               switch (reply->error()) {
[snip]
> +              
> this->ui->buttonBox->setStandardButtons(QDialogButtonBox::Cancel); +       
>        this->ui->txtResult->setHtml(QString(error));

No need to do that large switch. Just use reply->errorString().

> +       request.setRawHeader("User-Agent", "subsurface");

Please use UserSurvey::getUserAgent().

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel Open Source Technology Center
      PGP/GPG: 0x6EF45358; fingerprint:
      E067 918B B660 DBD1 105C  966C 33F5 F005 6EF4 5358



More information about the subsurface mailing list