DiveShare patch

Dirk Hohndel dirk at hohndel.org
Sun Oct 12 13:04:05 PDT 2014


On Sun, Oct 12, 2014 at 07:41:41PM +0200, Salvo Tomaselli wrote:
> Greetings,
> 
> I made this patch that adds a new export option, to diveshare.

Nice

> Dives can still be anonymous, but users can click a button and 
> go on a page where they have a secret string that they can paste 
> so that dives go to their account directly.
> 
> Let me know if this can be merged or needs some fixes.

I'd like to see a few changes... see below

> Feel free to test it, but make sure that you don't test it with 
> any data that you wouldn't want to be public.

I have NOT tested this, just read the code...

> diff --git a/qt-ui/diveshareexportdialog.cpp b/qt-ui/diveshareexportdialog.cpp
> new file mode 100644
> index 0000000..a44c2ee
> --- /dev/null
> +++ b/qt-ui/diveshareexportdialog.cpp
[...]
> +void DiveShareExportDialog::finishedSlot(QNetworkReply* reply) {
> +	this->ui->progressBar->setVisible(false);
> +	if (reply->error() != 0) {
> +		const char* error;
> +		switch (reply->error()) {
> +		case QNetworkReply::ConnectionRefusedError:
> +			error = "the remote server refused the connection (the server is not accepting requests)";
> +			break;
> +		case QNetworkReply::RemoteHostClosedError:
> +			error ="the remote server closed the connection prematurely, before the entire reply was received and processed";
> +			break;
> +		case QNetworkReply::HostNotFoundError:
> +			error ="the remote host name was not found (invalid hostname)";
> +			break;
> +		case QNetworkReply::TimeoutError:
> +			error ="the connection to the remote server timed out";
> +			break;

and lots and lots more...
do we need all these errors? I.e., can they all happen?
please mark all the texts for translation since they are user facing.

> +		case QNetworkReply::TemporaryNetworkFailureError:
> +			error ="the connection was broken due to disconnection from the network, however the system has initiated roaming to another access point. The request should be resubmitted and will be processed as soon as the connection is re-established.";

While we have no hard line length limit, this one seems a bit excessive...

> +void DiveShareExportDialog::doUpload() {
[...]
> +
> +	//generate json
> +	struct membuffer buf = { 0 };
> +	export_list(&buf, "/tmp",  this->exportSelected, false);

A path like this will work on Linux and Mac, but not on Windows

This is what jumped out at me after casually reading through the code.
I'll have to spend more time with some of the protocol stuff (or get
Thiago to review that part).

The reason I didn't pull it and said "let's fix it in future commits" is
that I worry that this is completely untested under Windows and I want to
make sure that we continue to have working "daily" builds on that
platform...

/D


More information about the subsurface mailing list