[Qt] add messages when handling local payment request files #3374

Diapolo commented at 10:21 AM on December 9, 2013: none

important for the open URI dialog to give users feedback when a file is invalid etc.

laanwj commented at 11:18 AM on December 9, 2013: member

I tested it when I did the pull for the "Open URI" dialog and file: handling was already working. The current URI code handles file: URIs transparently. Why so much extra code?

Diapolo commented at 11:39 AM on December 9, 2013: none

I have no idea, why it was working then ;). But try to chose an invalid file, current master doesn't show any warning or error. You are creating a bitcoin: URI with local file sheme in your dialog, that needs to be converted back so we can directly access the file and don't need any fetching via network.

laanwj commented at 11:42 AM on December 9, 2013: member

It doesn't do any "Fetching via network" when you specify a file: URI. The Qt URI handling code recognizes file: URIs and handles them transparently, as it should.

Better error reporting is nice but wouldn't this be possible without having to do manual parsing on them?

Diapolo commented at 12:18 PM on December 9, 2013: none

@laanwj Take a loot at the code, in handleURIOrFile() we are reaching fetchRequest() for files selected via the URI dialog, which are then going to be processed. This is avoided by my code, because it skips fetchRequest() for local file sheme in an URI. @TheBlueMatt Which Qt version has @BitcoinPullTester? QUrl::isLocalFile() was introduced with Qt 4.8, which seems missing and that is causing the error.

Diapolo commented at 1:45 PM on December 9, 2013: none

@laanwj

I verified that indeed the file: is handled by our code, but by the networking code path.

I have 2 suggestions:

Do it my way, and prevent fetchRequest() and netRequestFinished(), as local pr-files shouldn't take the network processing code flow IMHO.
Rework the open URI dialog to don't generate a bitcoin: URI for a local file but directly call handleURIOrFile() with a filename passed instead an URI.

laanwj commented at 1:53 PM on December 9, 2013: member

I would really like to keep it this way and handle files through the normal code path for URIs. I don't want to add a special exception for files if it works fine like this.

If you want to add better error handling that's good, but please find a way to fit it in the current mechanism (with the added bonus that it works for all URIs and not a subset).

Unnecessary complexity only makes it harder to test and understand code.

Diapolo commented at 3:06 PM on December 9, 2013: none

@laanwj I added some more error/warning messages and reworked existing ones to not be too net-specific, as local payment request files (although using net code) would otherwise confuse users.

Diapolo commented at 3:38 PM on December 9, 2013: none

Seems we have once more a @BitcoinPullTester problem ;).

[Qt] add messages when handling local payment request files

- important for the open URI dialog to give users feedback
  when a file is invalid etc.

bd70562f66

Diapolo commented at 10:06 AM on December 10, 2013: none

@laanwj Can we merge this, so we have time for translations updates before 0.9 RC phase, if you agree to the changes?

in src/qt/paymentserver.cpp:None in bd70562f66

 379 |              QByteArray temp;
 380 |              temp.append(uri.queryItemValue("r"));
 381 |              QString decoded = QUrl::fromPercentEncoding(temp);
 382 |              QUrl fetchUrl(decoded, QUrl::StrictMode);
 383 |  
 384 | -            qDebug() << "PaymentServer::handleURIOrFile : fetchRequest(" << fetchUrl << ")";

laanwj commented at 10:48 AM on December 10, 2013:

Hm why not just always log this? That the URI might be invalid doesn't mean it didn't go into fetchRequest(...).

Diapolo commented at 10:51 AM on December 10, 2013:

fetchRequest() is only called, if the URL is valid?

laanwj commented at 11:11 AM on December 10, 2013:

Ok right

BitcoinPullTester commented at 10:50 AM on December 10, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/bd70562f660552973dcf99e00c7897ae884661da for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

Diapolo commented at 8:48 AM on December 16, 2013: none

@laanwj Anything to add?

laanwj commented at 9:06 AM on December 16, 2013: member

No, nothing to add, the code changes look fine.

Do you have suggestions on how to test the new messages?

Diapolo commented at 10:15 AM on December 16, 2013: none

Try to open an invalid file via Open URI dialog or via webserver (bitcoin: URI). The others should be more uncommon, as an invalid URI check is likely to rarely get into handleUriOrFile(). The error in the if (QFile::exists(s)) check can only happen on Mac, because this works with files AFAIK. @laanwj Did you verify it's reporting errors now?

Diapolo commented at 1:15 PM on January 6, 2014: none

Ping @laanwj

Diapolo commented at 6:41 AM on January 9, 2014: none

@laanwj Too bad you didn't merge this before updating translations...

laanwj commented at 8:06 AM on January 9, 2014: member

I just pulled from Transifex, that's unaffected by any changes in the repository except the en master file which I didn't touch...

laanwj referenced this in commit f126973fd0 on Jan 13, 2014

laanwj merged this on Jan 13, 2014

laanwj closed this on Jan 13, 2014

Diapolo deleted the branch on Jan 14, 2014

Bushstar referenced this in commit 4e8f4ea202 on Apr 8, 2020

DrahtBot locked this on Sep 8, 2021