QR-Code code update #1078

Diapolo commented at 12:28 PM on April 11, 2012: none

fix for: https://bitcointalk.org/index.php?topic=73695.msg839804#msg839804
QR dialog window title is now only the BC address, because "label << address >>" was too large for the titlebar anyway
removed a dead line with debug code
coding style polish
moved declaration of QRCodeDialog *d; into the foreach {}

in src/qt/addressbookpage.cpp:None in e381fab5c3 outdated

 320 | +            title = QString("<< %1 >>").arg(address);
 321 |  
 322 | -        QRCodeDialog *d = new QRCodeDialog(title, address, label, tab == ReceivingTab, this);
 323 | -        d->show();
 324 | +        QRCodeDialog *dialog = new QRCodeDialog(title, address, label, tab == ReceivingTab, this);
 325 | +        dialog->show();

laanwj commented at 4:26 PM on April 11, 2012:

Never mind: it already has deleteonclose in the constructor, this is OK, no memory leak happens.

Diapolo commented at 8:26 PM on April 11, 2012:

Right, didn't see this and Qt GUI stuff is still sometimes weird for me :D.

in src/qt/qrcodedialog.cpp:None in e381fab5c3 outdated

  23 | @@ -24,7 +24,9 @@
  24 |      ui->lblAm1->setVisible(enableReq);
  25 |      ui->lblAm2->setVisible(enableReq);
  26 |  
  27 | -    ui->lnLabel->setText(label);
  28 | +    // don't display "(no label)" if there IS no label, as this is confusing in the QR dialog
  29 | +    if(label != tr("(no label)"))

laanwj commented at 4:31 PM on April 11, 2012:

Please don't use a magic string "(no label)" here. Why not simply use an empty string for "no label"?

in src/qt/addressbookpage.cpp:None in e381fab5c3 outdated

 309 | @@ -310,16 +310,14 @@ void AddressBookPage::on_showQRCode_clicked()
 310 |      QTableView *table = ui->tableView;
 311 |      QModelIndexList indexes = table->selectionModel()->selectedRows(AddressTableModel::Address);
 312 |  
 313 | -
 314 | -    QRCodeDialog *d;
 315 |      foreach (QModelIndex index, indexes)
 316 |      {
 317 |          QString address = index.data().toString(),
 318 |              label = index.sibling(index.row(), 0).data().toString(),

laanwj commented at 4:32 PM on April 11, 2012:

These should be index(...).data(Qt::EditRole). This will get the raw, unformatted data suitable for passing to other functions (and also get rid of "(no label)")

Diapolo commented at 9:48 PM on April 11, 2012:

I changed this with the last commit, would be nice if you could take a look. That GUI stuff is pretty new for me.

in src/qt/addressbookpage.cpp:None in e381fab5c3 outdated

 315 |      foreach (QModelIndex index, indexes)
 316 |      {
 317 |          QString address = index.data().toString(),
 318 |              label = index.sibling(index.row(), 0).data().toString(),
 319 | -            title = QString("%1 << %2 >>").arg(label).arg(address);
 320 | +            title = QString("<< %1 >>").arg(address);

laanwj commented at 4:40 PM on April 11, 2012:

As both address and label are passed in, the code to construct the title could be moved to QRCodeDialog constructor

Diapolo commented at 8:22 PM on April 11, 2012:

Good idea, will change this.

in src/qt/forms/qrcodedialog.ui:None in 9e2c4fed22 outdated

 112 | @@ -113,7 +113,7 @@
 113 |          <item>
 114 |           <layout class="QGridLayout" name="gridLayout">
 115 |            <item row="0" column="0">
 116 | -           <widget class="QLabel" name="label_3">
 117 | +           <widget class="QLabel" name="lblLabel">

laanwj commented at 5:35 AM on April 12, 2012:

There's no need to name "static" labels that will not accessed in the code, leaving them as label_X is fine... no need either to change it back though, it's fine.

Diapolo commented at 5:44 AM on April 12, 2012:

I love speaking names ... that's kind of a fetish I have ;), sorry ^^.

laanwj commented at 5:40 AM on April 12, 2012: member

Visual ACK

Diapolo commented at 5:46 AM on April 12, 2012: none

0.7, because it has to do with URIs ^^?

laanwj commented at 5:51 AM on April 12, 2012: member

It has to do with URLs but not "URL stuff that scares Gavin".

And this fixes a crash bug reported on the forums? I guess it should go into 0.6.1 then...

Diapolo commented at 6:21 AM on April 12, 2012: none

This fixes a glitch with a payment request and the amount set, the crash observed in the forum was with 0.6 RC2 I can't reproduce this, but think as most changes here are very tiny it should go into 0.6.1.

laanwj commented at 8:00 AM on April 12, 2012: member

From what I understand he performs a DOS attack on the QR generation code. By pasting more and more text into the label field he eventually runs out of memory or some other limit.

Maybe we should enforce a sanity limit on the URL length (256?) and above that show an empty image.

I could be completely wrong.

Diapolo commented at 10:15 AM on April 12, 2012: none

An URI limit of <= 255 would be a good idea.

Diapolo commented at 4:40 PM on April 12, 2012: none

Added URI length limit and info message to prevent a DoS against the dialog window.

laanwj commented at 5:49 PM on April 12, 2012: member

Works well. However, the info message is wider than the QR dialog, can you enable wordWrap on lblQRCode in the designer? Alternatively, paste this into the .ui file under <widget class="QLabel" name="lblQRCode">:

 <property name="wordWrap">
  <bool>true</bool>
 </property>

Diapolo commented at 6:22 PM on April 12, 2012: none

@laanwj Done!

laanwj commented at 6:31 PM on April 12, 2012: member

Great, I intend to merge this asap, however seems sje397 and gavin's commit somehow ended up on this pull request?

Diapolo commented at 6:33 PM on April 12, 2012: none

ouch ... a rebase-mess sorry will cleanup ^^

fixed amount part of URI in QR-Codes / removed (no label) string if we have NO label / coding style updates / removed an unused variable 9e0dba8c17

updated to reflect pull-request suggestions / renamed some GUI elements 1e8c62b29c

limit length of generated URI to 255 chars to prevent a DoS against the QR-Code dialog b1a99c3a1f

enable wordWrap on lblQRCode / small code comment change 7261945eb5

Diapolo commented at 6:36 PM on April 12, 2012: none

fixed

laanwj referenced this in commit d844cb58a8 on Apr 12, 2012

laanwj merged this on Apr 12, 2012

laanwj closed this on Apr 12, 2012

coblee referenced this in commit cdb2ec86a3 on Jul 17, 2012

sanch0panza referenced this in commit 2841aff90e on May 17, 2018

lateminer referenced this in commit 110aa9c83d on May 6, 2020

DrahtBot locked this on Sep 8, 2021