From e6d230056202b05a27f187dff2648eb5c76fcfee Mon Sep 17 00:00:00 2001 From: Philip Kaufmann Date: Tue, 8 Jan 2013 08:17:58 +0100 Subject: [PATCH] Bitcoin-Qt: fix known addressbook bugs - add qSort() for cachedAddressTable, as qLowerBound() and qUpperBound() require the list to be in ascending order (see http://harmattan-dev.nokia.com/docs/library/html/qt4/qtalgorithms.html#qLowerBound) - add a new check in AddressTableModel::setData() to just return, when no changes were made to a label or an address (prevents entry duplication issue) - remove "rec->label = value.toString();" from AddressTableModel::setData() as the label gets updated by AddressTablePriv::updateEntry() anyway (seems @sipa added this line via https://github.com/bitcoin/bitcoin/commit/1025440184ef100a22d07c7bb543ee45cf169d64#L6R225) - add another new check in AddressTableModel::setData() to just return, if a duplicate address was found (prevents address overwrite) - add a new check to EditAddressDialog::setModel() to prevent setting an invalid model - re-work the switch-case statement in AddressTableModel::accept() to always break (as return get's called anyway) and order the list to match the enum definition - make accept() in editaddressdialog.h a public slot, which it should be - misc small coding style changes --- src/qt/addresstablemodel.cpp | 36 ++++++++++++++++++++++++++++-------- src/qt/addresstablemodel.h | 23 ++++++++++++----------- src/qt/editaddressdialog.cpp | 30 +++++++++++++++++++----------- src/qt/editaddressdialog.h | 8 +++++--- 4 files changed, 64 insertions(+), 33 deletions(-) diff --git a/src/qt/addresstablemodel.cpp b/src/qt/addresstablemodel.cpp index e65d3915..03b09cdc 100644 --- a/src/qt/addresstablemodel.cpp +++ b/src/qt/addresstablemodel.cpp @@ -69,6 +69,8 @@ public: QString::fromStdString(address.ToString()))); } } + // qLowerBound() and qUpperBound() require our cachedAddressTable list to be sorted in asc order + qSort(cachedAddressTable.begin(), cachedAddressTable.end(), AddressTableEntryLessThan()); } void updateEntry(const QString &address, const QString &label, bool isMine, int status) @@ -208,7 +210,7 @@ QVariant AddressTableModel::data(const QModelIndex &index, int role) const return QVariant(); } -bool AddressTableModel::setData(const QModelIndex & index, const QVariant & value, int role) +bool AddressTableModel::setData(const QModelIndex &index, const QVariant &value, int role) { if(!index.isValid()) return false; @@ -221,18 +223,36 @@ bool AddressTableModel::setData(const QModelIndex & index, const QVariant & valu switch(index.column()) { case Label: + // Do nothing, if old label == new label + if(rec->label == value.toString()) + { + editStatus = NO_CHANGES; + return false; + } wallet->SetAddressBookName(CBitcoinAddress(rec->address.toStdString()).Get(), value.toString().toStdString()); - rec->label = value.toString(); break; case Address: + // Do nothing, if old address == new address + if(CBitcoinAddress(rec->address.toStdString()) == CBitcoinAddress(value.toString().toStdString())) + { + editStatus = NO_CHANGES; + return false; + } // Refuse to set invalid address, set error status and return false - if(!walletModel->validateAddress(value.toString())) + else if(!walletModel->validateAddress(value.toString())) { editStatus = INVALID_ADDRESS; return false; } + // Check for duplicate addresses to prevent accidental deletion of addresses, if you try + // to paste an existing address over another address (with a different label) + else if(wallet->mapAddressBook.count(CBitcoinAddress(value.toString().toStdString()).Get())) + { + editStatus = DUPLICATE_ADDRESS; + return false; + } // Double-check that we're not overwriting a receiving address - if(rec->type == AddressTableEntry::Sending) + else if(rec->type == AddressTableEntry::Sending) { { LOCK(wallet->cs_wallet); @@ -244,7 +264,6 @@ bool AddressTableModel::setData(const QModelIndex & index, const QVariant & valu } break; } - return true; } return false; @@ -262,7 +281,7 @@ QVariant AddressTableModel::headerData(int section, Qt::Orientation orientation, return QVariant(); } -Qt::ItemFlags AddressTableModel::flags(const QModelIndex & index) const +Qt::ItemFlags AddressTableModel::flags(const QModelIndex &index) const { if(!index.isValid()) return 0; @@ -279,7 +298,7 @@ Qt::ItemFlags AddressTableModel::flags(const QModelIndex & index) const return retval; } -QModelIndex AddressTableModel::index(int row, int column, const QModelIndex & parent) const +QModelIndex AddressTableModel::index(int row, int column, const QModelIndex &parent) const { Q_UNUSED(parent); AddressTableEntry *data = priv->index(row); @@ -345,6 +364,7 @@ QString AddressTableModel::addRow(const QString &type, const QString &label, con { return QString(); } + // Add entry { LOCK(wallet->cs_wallet); @@ -353,7 +373,7 @@ QString AddressTableModel::addRow(const QString &type, const QString &label, con return QString::fromStdString(strAddress); } -bool AddressTableModel::removeRows(int row, int count, const QModelIndex & parent) +bool AddressTableModel::removeRows(int row, int count, const QModelIndex &parent) { Q_UNUSED(parent); AddressTableEntry *rec = priv->index(row); diff --git a/src/qt/addresstablemodel.h b/src/qt/addresstablemodel.h index 42974e3e..ae3e3b2f 100644 --- a/src/qt/addresstablemodel.h +++ b/src/qt/addresstablemodel.h @@ -29,26 +29,27 @@ public: /** Return status of edit/insert operation */ enum EditStatus { - OK, - INVALID_ADDRESS, /**< Unparseable address */ - DUPLICATE_ADDRESS, /**< Address already in address book */ - WALLET_UNLOCK_FAILURE, /**< Wallet could not be unlocked to create new receiving address */ - KEY_GENERATION_FAILURE /**< Generating a new public key for a receiving address failed */ + OK, /**< Everything ok */ + NO_CHANGES, /**< No changes were made during edit operation */ + INVALID_ADDRESS, /**< Unparseable address */ + DUPLICATE_ADDRESS, /**< Address already in address book */ + WALLET_UNLOCK_FAILURE, /**< Wallet could not be unlocked to create new receiving address */ + KEY_GENERATION_FAILURE /**< Generating a new public key for a receiving address failed */ }; - static const QString Send; /**< Specifies send address */ - static const QString Receive; /**< Specifies receive address */ + static const QString Send; /**< Specifies send address */ + static const QString Receive; /**< Specifies receive address */ /** @name Methods overridden from QAbstractTableModel @{*/ int rowCount(const QModelIndex &parent) const; int columnCount(const QModelIndex &parent) const; QVariant data(const QModelIndex &index, int role) const; - bool setData(const QModelIndex & index, const QVariant & value, int role); + bool setData(const QModelIndex &index, const QVariant &value, int role); QVariant headerData(int section, Qt::Orientation orientation, int role) const; - QModelIndex index(int row, int column, const QModelIndex & parent) const; - bool removeRows(int row, int count, const QModelIndex & parent = QModelIndex()); - Qt::ItemFlags flags(const QModelIndex & index) const; + QModelIndex index(int row, int column, const QModelIndex &parent) const; + bool removeRows(int row, int count, const QModelIndex &parent = QModelIndex()); + Qt::ItemFlags flags(const QModelIndex &index) const; /*@}*/ /* Add an address to the model. diff --git a/src/qt/editaddressdialog.cpp b/src/qt/editaddressdialog.cpp index 0d88aa47..5cfcb34b 100644 --- a/src/qt/editaddressdialog.cpp +++ b/src/qt/editaddressdialog.cpp @@ -25,7 +25,7 @@ EditAddressDialog::EditAddressDialog(Mode mode, QWidget *parent) : break; case EditReceivingAddress: setWindowTitle(tr("Edit receiving address")); - ui->addressEdit->setDisabled(true); + ui->addressEdit->setEnabled(false); break; case EditSendingAddress: setWindowTitle(tr("Edit sending address")); @@ -44,6 +44,9 @@ EditAddressDialog::~EditAddressDialog() void EditAddressDialog::setModel(AddressTableModel *model) { this->model = model; + if(!model) + return; + mapper->setModel(model); mapper->addMapping(ui->labelEdit, AddressTableModel::Label); mapper->addMapping(ui->addressEdit, AddressTableModel::Address); @@ -58,6 +61,7 @@ bool EditAddressDialog::saveCurrentRow() { if(!model) return false; + switch(mode) { case NewReceivingAddress: @@ -82,35 +86,39 @@ void EditAddressDialog::accept() { if(!model) return; + if(!saveCurrentRow()) { switch(model->getEditStatus()) { - case AddressTableModel::DUPLICATE_ADDRESS: - QMessageBox::warning(this, windowTitle(), - tr("The entered address \"%1\" is already in the address book.").arg(ui->addressEdit->text()), - QMessageBox::Ok, QMessageBox::Ok); + case AddressTableModel::OK: + // Failed with unknown reason. Just reject. + break; + case AddressTableModel::NO_CHANGES: + // No changes were made during edit operation. Just reject. break; case AddressTableModel::INVALID_ADDRESS: QMessageBox::warning(this, windowTitle(), tr("The entered address \"%1\" is not a valid Bitcoin address.").arg(ui->addressEdit->text()), QMessageBox::Ok, QMessageBox::Ok); - return; + break; + case AddressTableModel::DUPLICATE_ADDRESS: + QMessageBox::warning(this, windowTitle(), + tr("The entered address \"%1\" is already in the address book.").arg(ui->addressEdit->text()), + QMessageBox::Ok, QMessageBox::Ok); + break; case AddressTableModel::WALLET_UNLOCK_FAILURE: QMessageBox::critical(this, windowTitle(), tr("Could not unlock wallet."), QMessageBox::Ok, QMessageBox::Ok); - return; + break; case AddressTableModel::KEY_GENERATION_FAILURE: QMessageBox::critical(this, windowTitle(), tr("New key generation failed."), QMessageBox::Ok, QMessageBox::Ok); - return; - case AddressTableModel::OK: - // Failed with unknown reason. Just reject. break; - } + } return; } QDialog::accept(); diff --git a/src/qt/editaddressdialog.h b/src/qt/editaddressdialog.h index 7ec053f1..0e4183bd 100644 --- a/src/qt/editaddressdialog.h +++ b/src/qt/editaddressdialog.h @@ -27,15 +27,17 @@ public: }; explicit EditAddressDialog(Mode mode, QWidget *parent = 0); - ~EditAddressDialog(); + ~EditAddressDialog(); void setModel(AddressTableModel *model); void loadRow(int row); - void accept(); - QString getAddress() const; void setAddress(const QString &address); + +public slots: + void accept(); + private: bool saveCurrentRow();