Qt: Enable searching by transaction id #11395

luke-jr commented at 4:20 AM on September 25, 2017: member

No description provided.

fanquake added the label GUI on Sep 25, 2017

meshcollider commented at 8:15 AM on September 25, 2017: contributor

utACK https://github.com/bitcoin/bitcoin/pull/11395/commits/3b35414b38be2b013be672e1ea727f67d1e42af2

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

in src/qt/transactionfilterproxy.cpp:55 in 3b35414b38 outdated

  51 | @@ -51,8 +52,11 @@ bool TransactionFilterProxy::filterAcceptsRow(int sourceRow, const QModelIndex &
  52 |          return false;
  53 |      if(datetime < dateFrom || datetime > dateTo)
  54 |          return false;
  55 | -    if (!address.contains(searchString, Qt::CaseInsensitive) && !label.contains(searchString, Qt::CaseInsensitive))
  56 | +    if (!address.contains(searchString, Qt::CaseInsensitive)

promag commented at 9:10 AM on September 25, 2017:

Avoid unnecessary code execution?

if (!searchString.isEmpty()) {
    ...
}

luke-jr commented at 7:04 PM on September 25, 2017:

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 commented at 8:25 PM on September 25, 2017:

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

in src/qt/transactionfilterproxy.cpp:39 in 3b35414b38 outdated

  35 | @@ -36,6 +36,7 @@ bool TransactionFilterProxy::filterAcceptsRow(int sourceRow, const QModelIndex &
  36 |      int type = index.data(TransactionTableModel::TypeRole).toInt();
  37 |      QDateTime datetime = index.data(TransactionTableModel::DateRole).toDateTime();
  38 |      bool involvesWatchAddress = index.data(TransactionTableModel::WatchonlyRole).toBool();
  39 | +    QString txid = index.data(TransactionTableModel::TxIDRole).toString();

promag commented at 9:11 AM on September 25, 2017:

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

in src/qt/transactionfilterproxy.cpp:56 in 3b35414b38 outdated

  51 | @@ -51,8 +52,11 @@ bool TransactionFilterProxy::filterAcceptsRow(int sourceRow, const QModelIndex &
  52 |          return false;
  53 |      if(datetime < dateFrom || datetime > dateTo)
  54 |          return false;
  55 | -    if (!address.contains(searchString, Qt::CaseInsensitive) && !label.contains(searchString, Qt::CaseInsensitive))
  56 | +    if (!address.contains(searchString, Qt::CaseInsensitive)
  57 | +     && !  label.contains(searchString, Qt::CaseInsensitive)

promag commented at 9:12 AM on September 25, 2017:

Nit, drop alignments?

luke-jr commented at 7:05 PM on September 25, 2017:

Why?

promag commented at 8:27 PM on September 25, 2017:

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)) {

in src/qt/transactionfilterproxy.cpp:69 in 466fb0e4db outdated

  65 | @@ -66,9 +66,9 @@ void TransactionFilterProxy::setDateRange(const QDateTime &from, const QDateTime
  66 |      invalidateFilter();
  67 |  }
  68 |  
  69 | -void TransactionFilterProxy::setAddressPrefix(const QString &_addrPrefix)
  70 | +void TransactionFilterProxy::setSearchString(const QString &_searchString)

promag commented at 9:14 AM on September 25, 2017:

Nit, const QString& search_string.

in src/qt/transactionfilterproxy.cpp:23 in 466fb0e4db outdated

  19 | @@ -20,7 +20,7 @@ TransactionFilterProxy::TransactionFilterProxy(QObject *parent) :
  20 |      QSortFilterProxyModel(parent),
  21 |      dateFrom(MIN_DATE),
  22 |      dateTo(MAX_DATE),
  23 | -    addrPrefix(),
  24 | +    searchString(),

promag commented at 9:15 AM on September 25, 2017:

New convention m_search_string?

in src/qt/transactionview.cpp:308 in 466fb0e4db outdated

 304 | @@ -305,11 +305,11 @@ void TransactionView::chooseWatchonly(int idx)
 305 |          (TransactionFilterProxy::WatchOnlyFilter)watchOnlyWidget->itemData(idx).toInt());
 306 |  }
 307 |  
 308 | -void TransactionView::changedPrefix(const QString &prefix)
 309 | +void TransactionView::changedSearch(const QString &prefix)

promag commented at 9:16 AM on September 25, 2017:

Renameprefix variable too?

in src/qt/transactionview.cpp:312 in 466fb0e4db outdated

 309 | +void TransactionView::changedSearch(const QString &prefix)
 310 |  {
 311 |      if(!transactionProxyModel)
 312 |          return;
 313 | -    transactionProxyModel->setAddressPrefix(prefix);
 314 | +    transactionProxyModel->setSearchString(prefix);

promag commented at 9:18 AM on September 25, 2017:

Suggestion, sanitize input, like trim?

luke-jr commented at 7:06 PM on September 25, 2017:

Why?

promag commented at 8:31 PM on September 25, 2017:

Only a suggestion really.

in src/qt/transactionfilterproxy.cpp:71 in 466fb0e4db outdated

  65 | @@ -66,9 +66,9 @@ void TransactionFilterProxy::setDateRange(const QDateTime &from, const QDateTime
  66 |      invalidateFilter();
  67 |  }
  68 |  
  69 | -void TransactionFilterProxy::setAddressPrefix(const QString &_addrPrefix)
  70 | +void TransactionFilterProxy::setSearchString(const QString &_searchString)
  71 |  {
  72 | -    this->addrPrefix = _addrPrefix;
  73 | +    this->searchString = _searchString;

promag commented at 9:18 AM on September 25, 2017:

Nit:

if (this->m_search_string == search_string) return;

luke-jr commented at 7:05 PM on September 25, 2017:

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

promag commented at 8:31 PM on September 25, 2017:

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 commented at 8:32 PM on September 25, 2017:

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

luke-jr commented at 9:11 PM on September 25, 2017:

Ah, good points.

luke-jr commented at 11:30 PM on September 25, 2017:

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 commented at 11:40 PM on September 25, 2017:

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)?

promag commented at 9:18 AM on September 25, 2017: member

Concept ACK.

luke-jr force-pushed on Sep 25, 2017

in src/qt/transactionfilterproxy.cpp:56 in 93f0dfd33d outdated

  51 | @@ -51,8 +52,11 @@ bool TransactionFilterProxy::filterAcceptsRow(int sourceRow, const QModelIndex &
  52 |          return false;
  53 |      if(datetime < dateFrom || datetime > dateTo)
  54 |          return false;
  55 | -    if (!address.contains(addrPrefix, Qt::CaseInsensitive) && !label.contains(addrPrefix, Qt::CaseInsensitive))
  56 | +    if (!address.contains(m_search_string, Qt::CaseInsensitive) &&
  57 | +        !  label.contains(m_search_string, Qt::CaseInsensitive) &&

promag commented at 11:38 PM on September 25, 2017:

clang-format will remove these whitespaces.

luke-jr commented at 12:39 AM on September 26, 2017:

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

in src/qt/transactionfilterproxy.cpp:73 in f2eb2d4a35 outdated

  65 | @@ -66,9 +66,9 @@ void TransactionFilterProxy::setDateRange(const QDateTime &from, const QDateTime
  66 |      invalidateFilter();
  67 |  }
  68 |  
  69 | -void TransactionFilterProxy::setAddressPrefix(const QString &_addrPrefix)
  70 | +void TransactionFilterProxy::setSearchString(const QString &search_string)

promag commented at 11:41 PM on September 25, 2017:

const QString& search_string.

in src/qt/transactionfilterproxy.h:38 in f2eb2d4a35 outdated

  34 | @@ -35,7 +35,7 @@ class TransactionFilterProxy : public QSortFilterProxyModel
  35 |      };
  36 |  
  37 |      void setDateRange(const QDateTime &from, const QDateTime &to);
  38 | -    void setAddressPrefix(const QString &addrPrefix);
  39 | +    void setSearchString(const QString &);

promag commented at 11:42 PM on September 25, 2017:

const QString&.

meshcollider commented at 11:12 AM on November 13, 2017:

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

in src/qt/transactionview.cpp:308 in f2eb2d4a35 outdated

 304 | @@ -305,11 +305,11 @@ void TransactionView::chooseWatchonly(int idx)
 305 |          (TransactionFilterProxy::WatchOnlyFilter)watchOnlyWidget->itemData(idx).toInt());
 306 |  }
 307 |  
 308 | -void TransactionView::changedPrefix(const QString &prefix)
 309 | +void TransactionView::changedSearch(const QString &search_string)

promag commented at 11:43 PM on September 25, 2017:

Nit, const QString& search_string.

in src/qt/transactionview.h:69 in f2eb2d4a35 outdated

  65 | @@ -66,7 +66,7 @@ class TransactionView : public QWidget
  66 |      QComboBox *dateWidget;
  67 |      QComboBox *typeWidget;
  68 |      QComboBox *watchOnlyWidget;
  69 | -    QLineEdit *addressWidget;
  70 | +    QLineEdit *search_widget;

promag commented at 11:43 PM on September 25, 2017:

Nit, QLineEdit* m_search_widget.

in src/qt/transactionview.h:113 in f2eb2d4a35 outdated

 109 | @@ -110,7 +110,7 @@ public Q_SLOTS:
 110 |      void chooseDate(int idx);
 111 |      void chooseType(int idx);
 112 |      void chooseWatchonly(int idx);
 113 | -    void changedPrefix(const QString &prefix);
 114 | +    void changedSearch(const QString &search_string);

promag commented at 11:44 PM on September 25, 2017:

Nit, const QString& search_string.

in src/qt/transactionfilterproxy.cpp:55 in 93f0dfd33d outdated

  51 | @@ -51,8 +52,11 @@ bool TransactionFilterProxy::filterAcceptsRow(int sourceRow, const QModelIndex &
  52 |          return false;
  53 |      if(datetime < dateFrom || datetime > dateTo)
  54 |          return false;
  55 | -    if (!address.contains(addrPrefix, Qt::CaseInsensitive) && !label.contains(addrPrefix, Qt::CaseInsensitive))
  56 | +    if (!address.contains(m_search_string, Qt::CaseInsensitive) &&

promag commented at 11:50 PM on September 25, 2017:

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 commented at 12:34 AM on September 26, 2017:

Sounds like more complication for little value.

promag commented at 12:23 PM on September 26, 2017:

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

promag commented at 12:22 PM on September 26, 2017: member

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 commented at 7:26 PM on September 26, 2017: member

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 commented at 10:02 PM on September 26, 2017: contributor

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

promag commented at 8:02 AM on October 7, 2017: member

Needs rebase.

Sjors commented at 1:45 PM on November 9, 2017: member

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: <img width="845" alt="schermafbeelding 2017-11-09 om 14 40 31" src="https://user-images.githubusercontent.com/10217/32608220-f8c118be-c55b-11e7-8903-2a4f78a97731.png">

promag commented at 2:04 PM on November 9, 2017: member

@luke-jr ping.

promag commented at 2:08 PM on November 9, 2017: member

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.

Qt: Rename confusingly-named "address prefix" to "search string" b1f634242e

Qt: Avoid invalidating the search filter, when it doesn't really change c407c61c5b

Qt: Enable searching by transaction id eac2abca02

luke-jr commented at 11:50 AM on November 10, 2017: member

Rebased

luke-jr force-pushed on Nov 10, 2017

promag commented at 12:34 PM on November 12, 2017: member

Restarted travis job due to unrelated error.

promag commented at 12:36 PM on November 12, 2017: member

utACK.

meshcollider commented at 11:13 AM on November 13, 2017: contributor

re-utACK https://github.com/bitcoin/bitcoin/commit/c407c61c5bd7a41dae23d280644d46a9883de6ae

jonasschnelli approved

jonasschnelli commented at 11:50 PM on November 29, 2017: contributor

Tested ACK c407c61c5bd7a41dae23d280644d46a9883de6ae

jonasschnelli merged this on Nov 29, 2017

jonasschnelli closed this on Nov 29, 2017

jonasschnelli referenced this in commit 38d31f95d3 on Nov 29, 2017

jasonbcox referenced this in commit 524ce61e18 on Aug 16, 2019

jonspock referenced this in commit 5e0d0bb64e on Dec 8, 2019

jonspock referenced this in commit 694fec4d1b on Dec 8, 2019

proteanx referenced this in commit 64d1b55f02 on Dec 12, 2019

PastaPastaPasta referenced this in commit b54c46b0b9 on Jan 17, 2020

PastaPastaPasta referenced this in commit 2626f272a0 on Jan 22, 2020

PastaPastaPasta referenced this in commit 43b1c75374 on Jan 22, 2020

PastaPastaPasta referenced this in commit be67f38e76 on Jan 29, 2020

PastaPastaPasta referenced this in commit 26cbc9be3b on Jan 29, 2020

PastaPastaPasta referenced this in commit 4f2fcbd1a3 on Jan 29, 2020

ckti referenced this in commit 7c20c858d3 on Mar 28, 2021

gades referenced this in commit c2aae9618a on Jun 26, 2021

DrahtBot locked this on Sep 8, 2021