/bitcoin

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Qt: Enable searching by transaction id #11395

Open
wants to merge 3 commits into
from
+20 −15

Conversation

Reviewers

@MeshCollider MeshCollider

@promag promag

Assignees
No one assigned
Labels
GUI
Projects
None yet
Milestone
No milestone
6 participants
@luke-jr @promag @jonasschnelli @Sjors @MeshCollider @fanquake
@luke-jr
Member

luke-jr commented Sep 25, 2017

No description provided.

@fanquake fanquake added the GUI label Sep 25, 2017

@MeshCollider

MeshCollider reviewed Sep 25, 2017 • edited
View changes

utACK 3b35414

nit: use snake_case since you are refactoring the variable names anyway

src/qt/transactionfilterproxy.cpp
@@ -36,6 +36,7 @@ bool TransactionFilterProxy::filterAcceptsRow(int sourceRow, const QModelIndex &
int type = index.data(TransactionTableModel::TypeRole).toInt();
QDateTime datetime = index.data(TransactionTableModel::DateRole).toDateTime();
bool involvesWatchAddress = index.data(TransactionTableModel::WatchonlyRole).toBool();
+ QString txid = index.data(TransactionTableModel::TxIDRole).toString();
@promag

promag Sep 25, 2017

Contributor

Nit, declare after label (sorted and follows the same test order below)?

src/qt/transactionfilterproxy.cpp
@@ -20,7 +20,7 @@ TransactionFilterProxy::TransactionFilterProxy(QObject *parent) :
QSortFilterProxyModel(parent),
dateFrom(MIN_DATE),
dateTo(MAX_DATE),
- addrPrefix(),
+ searchString(),
@promag

promag Sep 25, 2017

Contributor

New convention m_search_string?

src/qt/transactionfilterproxy.cpp
@@ -51,8 +52,11 @@ bool TransactionFilterProxy::filterAcceptsRow(int sourceRow, const QModelIndex &
return false;
if(datetime < dateFrom || datetime > dateTo)
return false;
- if (!address.contains(searchString, Qt::CaseInsensitive) && !label.contains(searchString, Qt::CaseInsensitive))
+ if (!address.contains(searchString, Qt::CaseInsensitive)
@promag

promag Sep 25, 2017

Contributor

Avoid unnecessary code execution?

if (!searchString.isEmpty()) {
    ...
}
@luke-jr

luke-jr Sep 25, 2017

Member

address.contains("") will immediately return true, so the other two clauses won't get executed. I don't know that we need to over-optimise here.

@promag

promag Sep 25, 2017

Contributor

Ok, not so immediately as isEmpty but agree with you.

src/qt/transactionfilterproxy.cpp
@@ -51,8 +52,11 @@ bool TransactionFilterProxy::filterAcceptsRow(int sourceRow, const QModelIndex &
return false;
if(datetime < dateFrom || datetime > dateTo)
return false;
- if (!address.contains(searchString, Qt::CaseInsensitive) && !label.contains(searchString, Qt::CaseInsensitive))
+ if (!address.contains(searchString, Qt::CaseInsensitive)
+ && ! label.contains(searchString, Qt::CaseInsensitive)
@promag

promag Sep 25, 2017

Contributor

Nit, drop alignments?

@luke-jr

luke-jr Sep 25, 2017

Member

Why?

@promag

promag Sep 25, 2017

Contributor

As it is clang-format makes it one line. This is preserved by clang-format:

    if (!address.contains(searchString, Qt::CaseInsensitive) &&
        !label.contains(searchString, Qt::CaseInsensitive) &&
        !txid.contains(searchString, Qt::CaseInsensitive)) {
src/qt/transactionfilterproxy.cpp
@@ -66,9 +66,9 @@ void TransactionFilterProxy::setDateRange(const QDateTime &from, const QDateTime
invalidateFilter();
}
-void TransactionFilterProxy::setAddressPrefix(const QString &_addrPrefix)
+void TransactionFilterProxy::setSearchString(const QString &_searchString)
@promag

promag Sep 25, 2017

Contributor

Nit, const QString& search_string.

src/qt/transactionfilterproxy.cpp
{
- this->addrPrefix = _addrPrefix;
+ this->searchString = _searchString;
@promag

promag Sep 25, 2017

Contributor

Nit:

if (this->m_search_string == search_string) return;
@luke-jr

luke-jr Sep 25, 2017

Member

I think that would actually make performance worse...?

@promag

promag Sep 25, 2017

Contributor

As it is now yes because QLineEdit doesn't emit unnecessary signals. But from this method perspective IMO this makes sense, the cost of the condition is way cheaper than invalidating the filter.

@promag

promag Sep 25, 2017

Contributor

BTW, we could lower case the stored m_search_string to make the filter faster.

@luke-jr

luke-jr Sep 25, 2017

Member

Ah, good points.

@luke-jr

luke-jr Sep 25, 2017

Member

On second thought, we should never get to this point if the string hasn't changed, and lowercasing the search string doesn't make the filter faster because label/address aren't lowercased.

@promag

promag Sep 25, 2017

Contributor

Imagine if pressing the ESC key erases the search string, pressing it the 2nd time would do nothing if the check is there.

So why .contains(..., Qt::CaseInsensitive)?

src/qt/transactionview.cpp
@@ -305,11 +305,11 @@ void TransactionView::chooseWatchonly(int idx)
(TransactionFilterProxy::WatchOnlyFilter)watchOnlyWidget->itemData(idx).toInt());
}
-void TransactionView::changedPrefix(const QString &prefix)
+void TransactionView::changedSearch(const QString &prefix)
@promag

promag Sep 25, 2017

Contributor

Renameprefix variable too?

src/qt/transactionview.cpp
{
if(!transactionProxyModel)
return;
- transactionProxyModel->setAddressPrefix(prefix);
+ transactionProxyModel->setSearchString(prefix);
@promag

promag Sep 25, 2017

Contributor

Suggestion, sanitize input, like trim?

@luke-jr

luke-jr Sep 25, 2017

Member

Why?

@promag

promag Sep 25, 2017

Contributor

Only a suggestion really.

src/qt/transactionfilterproxy.cpp
@@ -51,8 +52,11 @@ bool TransactionFilterProxy::filterAcceptsRow(int sourceRow, const QModelIndex &
return false;
if(datetime < dateFrom || datetime > dateTo)
return false;
- if (!address.contains(addrPrefix, Qt::CaseInsensitive) && !label.contains(addrPrefix, Qt::CaseInsensitive))
+ if (!address.contains(m_search_string, Qt::CaseInsensitive) &&
+ ! label.contains(m_search_string, Qt::CaseInsensitive) &&
@promag

promag Sep 25, 2017

Contributor

clang-format will remove these whitespaces.

@luke-jr

luke-jr Sep 26, 2017

Member

I consider this a shortcoming of clang-format, then. I don't consider the remaining nits as things that should be changed.

src/qt/transactionfilterproxy.cpp
@@ -66,9 +66,9 @@ void TransactionFilterProxy::setDateRange(const QDateTime &from, const QDateTime
invalidateFilter();
}
-void TransactionFilterProxy::setAddressPrefix(const QString &_addrPrefix)
+void TransactionFilterProxy::setSearchString(const QString &search_string)
@promag

promag Sep 25, 2017

Contributor

const QString& search_string.

src/qt/transactionfilterproxy.h
@@ -35,7 +35,7 @@ class TransactionFilterProxy : public QSortFilterProxyModel
};
void setDateRange(const QDateTime &from, const QDateTime &to);
- void setAddressPrefix(const QString &addrPrefix);
+ void setSearchString(const QString &);
@promag

promag Sep 25, 2017

Contributor

const QString&.

@MeshCollider

MeshCollider Nov 13, 2017

Member

Nit: I prefer having the variable names in the header (all others in this header do too). Unsure if there's a general preference though

src/qt/transactionview.cpp
@@ -305,11 +305,11 @@ void TransactionView::chooseWatchonly(int idx)
(TransactionFilterProxy::WatchOnlyFilter)watchOnlyWidget->itemData(idx).toInt());
}
-void TransactionView::changedPrefix(const QString &prefix)
+void TransactionView::changedSearch(const QString &search_string)
@promag

promag Sep 25, 2017

Contributor

Nit, const QString& search_string.

src/qt/transactionview.h
@@ -66,7 +66,7 @@ class TransactionView : public QWidget
QComboBox *dateWidget;
QComboBox *typeWidget;
QComboBox *watchOnlyWidget;
- QLineEdit *addressWidget;
+ QLineEdit *search_widget;
@promag

promag Sep 25, 2017

Contributor

Nit, QLineEdit* m_search_widget.

src/qt/transactionview.h
@@ -110,7 +110,7 @@ public Q_SLOTS:
void chooseDate(int idx);
void chooseType(int idx);
void chooseWatchonly(int idx);
- void changedPrefix(const QString &prefix);
+ void changedSearch(const QString &search_string);
@promag

promag Sep 25, 2017

Contributor

Nit, const QString& search_string.

src/qt/transactionfilterproxy.cpp
@@ -51,8 +52,11 @@ bool TransactionFilterProxy::filterAcceptsRow(int sourceRow, const QModelIndex &
return false;
if(datetime < dateFrom || datetime > dateTo)
return false;
- if (!address.contains(addrPrefix, Qt::CaseInsensitive) && !label.contains(addrPrefix, Qt::CaseInsensitive))
+ if (!address.contains(m_search_string, Qt::CaseInsensitive) &&
@promag

promag Sep 25, 2017

Contributor

BTW, back to if (!m_search_string.isEmpty()) { ... }, it can also save querying the model for those properties:

if (!m_search_string_lowered_case.isEmpty()) {
    QString address = ...toLowerCase();
    ...
    return ...;

}
@luke-jr

luke-jr Sep 26, 2017

Member

Sounds like more complication for little value.

@promag

promag Sep 26, 2017

Contributor

Probably, maybe @lclc can bench the change in #11015 for his use case.

@promag
Contributor

promag commented Sep 26, 2017

I don't consider the remaining nits as things that should be changed.

These aren't significant changes so why not comply with developer notes?
@luke-jr
Member

luke-jr commented Sep 26, 2017

The developer notes don't say anything about which side * and & should hug, or the rest. Nitpicking such minor details seems like just a waste of time.
@jonasschnelli
Member

jonasschnelli commented Sep 26, 2017

Concept ACK (haven't looked at the code so far).
@promag
Contributor

promag commented Oct 7, 2017

Needs rebase.
@Sjors
Contributor

Sjors commented Nov 9, 2017

Concept ACK. It's a bit strange to search for a field that's not visible in the table. Adding (part of) the transaction hash as a column would be more consistent, but obviously a bigger change and perhaps a waste of horizontal space. However that could all be added later.

It works for me:
schermafbeelding 2017-11-09 om 14 40 31
@promag
Contributor

promag commented Nov 9, 2017

@luke-jr ping.
@promag
Contributor

promag commented Nov 9, 2017

It's a bit strange to search for a field that's not visible in the table

It is also strange to not have the txid in the "Transactions" view. 🙄 Agree it can be improved later.
@luke-jr
Member

luke-jr commented Nov 10, 2017

Rebased
@promag
Contributor

promag commented Nov 12, 2017

Restarted travis job due to unrelated error.
@promag
Contributor

promag commented Nov 12, 2017

utACK.
src/qt/transactionfilterproxy.h
@@ -35,7 +35,7 @@ class TransactionFilterProxy : public QSortFilterProxyModel
};
void setDateRange(const QDateTime &from, const QDateTime &to);
- void setAddressPrefix(const QString &addrPrefix);
+ void setSearchString(const QString &);
@MeshCollider

MeshCollider Nov 13, 2017

Member

Nit: I prefer having the variable names in the header (all others in this header do too). Unsure if there's a general preference though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment