[Qt] Add delay before filtering transactions #11015

lclc commented at 7:00 AM on August 9, 2017: contributor

As discussed in #3141.

This adds a QTimer pause of 200ms before start to filter so it should be possible to filter big data sets easier.

fanquake added the label GUI on Aug 9, 2017

in src/qt/transactionview.cpp:185 in 9943f49073 outdated

 179 | @@ -173,8 +180,10 @@ TransactionView::TransactionView(const PlatformStyle *platformStyle, QWidget *pa
 180 |      connect(dateWidget, SIGNAL(activated(int)), this, SLOT(chooseDate(int)));
 181 |      connect(typeWidget, SIGNAL(activated(int)), this, SLOT(chooseType(int)));
 182 |      connect(watchOnlyWidget, SIGNAL(activated(int)), this, SLOT(chooseWatchonly(int)));
 183 | -    connect(addressWidget, SIGNAL(textChanged(QString)), this, SLOT(changedPrefix(QString)));
 184 | -    connect(amountWidget, SIGNAL(textChanged(QString)), this, SLOT(changedAmount(QString)));
 185 | +    connect(addressWidget, SIGNAL(textChanged(QString)), this, SLOT(changedPrefix()));
 186 | +    connect(inputDelayPrefix, SIGNAL(timeout()), this, SLOT(filterByPrefix()));
 187 | +    connect(amountWidget, SIGNAL(textChanged(QString)), this, SLOT(changedAmount()));

promag commented at 7:12 AM on August 9, 2017:

Connect to start QTimer::start() slot instead, and configure the interval above.

in src/qt/transactionview.cpp:183 in 9943f49073 outdated

 179 | @@ -173,8 +180,10 @@ TransactionView::TransactionView(const PlatformStyle *platformStyle, QWidget *pa
 180 |      connect(dateWidget, SIGNAL(activated(int)), this, SLOT(chooseDate(int)));
 181 |      connect(typeWidget, SIGNAL(activated(int)), this, SLOT(chooseType(int)));
 182 |      connect(watchOnlyWidget, SIGNAL(activated(int)), this, SLOT(chooseWatchonly(int)));
 183 | -    connect(addressWidget, SIGNAL(textChanged(QString)), this, SLOT(changedPrefix(QString)));
 184 | -    connect(amountWidget, SIGNAL(textChanged(QString)), this, SLOT(changedAmount(QString)));
 185 | +    connect(addressWidget, SIGNAL(textChanged(QString)), this, SLOT(changedPrefix()));

promag commented at 7:13 AM on August 9, 2017:

Connect to start QTimer::start() slot instead, and configure the interval above.

in src/qt/transactionview.cpp:336 in 9943f49073 outdated

 333 |          return;
 334 | -    transactionProxyModel->setAddressPrefix(prefix);
 335 | +    transactionProxyModel->setAddressPrefix(addressWidget->text());
 336 | +}
 337 | +
 338 | +void TransactionView::changedAmount()

promag commented at 7:15 AM on August 9, 2017:

Can be removed.

promag commented at 7:16 AM on August 9, 2017: member

Should the interval be configurable? At least it's duplicated so have a const variable?

promag commented at 7:18 AM on August 9, 2017: member

Nit, reword commit and PR title to qt: Add delay before filtering transactions?

jonasschnelli commented at 7:30 AM on August 9, 2017: contributor

Concept ACK

lclc force-pushed on Aug 9, 2017

lclc renamed this:
~~[Qt] Search TX: Add delay before filtering~~
[Qt] Add delay before filtering transactions
on Aug 9, 2017

lclc commented at 9:00 AM on August 9, 2017: contributor

Thanks a lot for the fast review and suggestions for improvement @promag. I've updated the commit accordingly.

Should the interval be configurable?

I don't think many people want to change that. If we see 200ms is not optimal we can still change it later. Adding this as a config flag option would probably mostly add code that nobody uses in the end - but if more people think it's a good idea I can implement it.

sipa commented at 9:02 AM on August 9, 2017: member

Don't make things configurable if there is no advice for what to set it to.

in src/qt/transactionview.h:77 in 1a03f63e76 outdated

  69 | @@ -69,6 +70,12 @@ class TransactionView : public QWidget
  70 |      QLineEdit *addressWidget;
  71 |      QLineEdit *amountWidget;
  72 |  
  73 | +    QTimer *inputDelayPrefix;
  74 | +    QTimer *inputDelayAmount;
  75 | +
  76 | +    /* Delay before before filtering transactions in ms */
  77 | +    static const int inputFilterDelay = 200;

promag commented at 9:56 AM on August 9, 2017:

Move to .cpp?

in src/qt/transactionview.h:73 in 1a03f63e76 outdated

  69 | @@ -69,6 +70,12 @@ class TransactionView : public QWidget
  70 |      QLineEdit *addressWidget;
  71 |      QLineEdit *amountWidget;
  72 |  
  73 | +    QTimer *inputDelayPrefix;

promag commented at 9:56 AM on August 9, 2017:

The convention is to prefix with m_ in new code.

lclc commented at 10:45 AM on August 9, 2017:

I also like the m_ convention but I don't see it in any of the other Qt code. Is the plan to slowly change to m_ over time or maybe take the effort to change (part of) the full source code at ones? Two different styles in the same file is not so nice I think.

promag commented at 3:41 PM on August 9, 2017:

@lclc unfortunately there's mixed styles all over the place. Last time that was discussed, that I know of, is to use the defined convention in new or "touched" code.

lclc commented at 4:31 PM on August 9, 2017:

Ok, thanks. I'll update.

promag commented at 9:59 AM on August 9, 2017: member

Agree with @sipa. A variable is enough to add meaning to the duration.

lclc force-pushed on Aug 9, 2017

lclc commented at 7:08 PM on August 9, 2017: contributor

Improved as suggested by @promag.

in src/qt/transactionview.h:73 in 0587d2e36a outdated

  69 | @@ -69,6 +70,9 @@ class TransactionView : public QWidget
  70 |      QLineEdit *addressWidget;
  71 |      QLineEdit *amountWidget;
  72 |  
  73 | +    QTimer *m_inputDelayPrefix;

benma commented at 9:06 PM on August 9, 2017:

please use snake_case as per developer notes.

jonasschnelli commented at 7:52 AM on August 10, 2017:

To also drop my opinion here (though I don't care): Use the scheme of the surrounding code to not confuse new developers. New classes or complete rewrites may be different.

sipa commented at 7:57 AM on August 10, 2017:

From the developer notes:

When writing patches, favor the new style over attempting to mimick the surrounding style, except for move-only commits.

and about symbol naming:

These are preferred in new code, but are not required when doing so would need changes to significant pieces of existing code.

jonasschnelli commented at 8:11 AM on August 10, 2017:

Hmm.. I see. My advice is against the developer notes then. It just looked confusing to have two single instance variables with sneak case where the rest is without m_ and camel case. But I guess we have to start somewhere to adopt the style rules.

in src/qt/transactionview.h:119 in 0587d2e36a outdated

 115 | @@ -112,8 +116,8 @@ public Q_SLOTS:
 116 |      void chooseDate(int idx);
 117 |      void chooseType(int idx);
 118 |      void chooseWatchonly(int idx);
 119 | -    void changedPrefix(const QString &prefix);
 120 | -    void changedAmount(const QString &amount);
 121 | +    void changedPrefix();

benma commented at 9:10 PM on August 9, 2017:

can be moved to the private Q_SLOTS section. All the others can as well, mabye in a separate commit?

lclc commented at 6:58 PM on August 10, 2017:

I plan to look at Q_SLOTS in general in a separate PR. There are a few other cases in different files where some slots are public that wouldn't need to be.

benma commented at 7:34 PM on August 10, 2017:

+1

promag commented at 12:17 AM on August 19, 2017:

Agree, all is done in this PR context.

benma commented at 9:18 PM on August 9, 2017: none

ACK 0587d2e36a0a64535e8d296fb97fcb7a258637fe

I tested that the issue existed and that this PR fixes it.

laanwj commented at 5:59 PM on August 10, 2017: member

Concept ACK, thanks!

Don't make things configurable if there is no advice for what to set it to.

Indeed, just make a choice, there's no use in making this configurable. No one will bother and it will just add more things to test.

lclc force-pushed on Aug 10, 2017

lclc commented at 7:01 PM on August 10, 2017: contributor

Thanks a lot for clarifying, review and testing @benma. I changed the commit according to the developer notes.

benma commented at 7:35 PM on August 10, 2017: none

ACK 6d073a2d1c85ecc523d13c5f47514daa5de4757c

in src/qt/transactionview.h:73 in 6d073a2d1c outdated

  69 | @@ -69,6 +70,9 @@ class TransactionView : public QWidget
  70 |      QLineEdit *addressWidget;
  71 |      QLineEdit *amountWidget;
  72 |  
  73 | +    QTimer *m_input_delay_prefix;

promag commented at 7:41 AM on August 18, 2017:

There is no need to have these here..

promag commented at 12:15 AM on August 19, 2017:

Please remove.

promag commented at 7:42 AM on August 18, 2017: member

utACK 6d073a2.

jonasschnelli commented at 10:02 AM on August 18, 2017: contributor

I couldn't really test this (all the machines where to fast). The delay seems very short (with 200ms). Will the timer be reset when entering the next char <200ms (I think so otherwise this would not work)?. Maybe @promag can do a test?

promag commented at 10:26 AM on August 18, 2017: member

@jonasschnelli yes start() resets.

in src/qt/transactionview.cpp:117 in 6d073a2d1c outdated

 112 | @@ -112,6 +113,17 @@ TransactionView::TransactionView(const PlatformStyle *platformStyle, QWidget *pa
 113 |      amountWidget->setValidator(new QDoubleValidator(0, 1e20, 8, this));
 114 |      hlayout->addWidget(amountWidget);
 115 |  
 116 | +    /* Delay before before filtering transactions in ms */
 117 | +    static const int inputFilterDelay = 200;

promag commented at 12:14 AM on August 19, 2017:

Use snake case. Comment to indicate unit?

FWIW there is this global constant cc @sipa.

in src/qt/transactionview.cpp:341 in 6d073a2d1c outdated

 340 |  {
 341 |      if(!transactionProxyModel)
 342 |          return;
 343 |      CAmount amount_parsed = 0;
 344 | -    if(BitcoinUnits::parse(model->getOptionsModel()->getDisplayUnit(), amount, &amount_parsed))
 345 | +    if(BitcoinUnits::parse(model->getOptionsModel()->getDisplayUnit(), amountWidget->text(), &amount_parsed))

promag commented at 12:15 AM on August 19, 2017:

Missing space after if. Move below { to this line.

in src/qt/transactionview.h:74 in 6d073a2d1c outdated

  69 | @@ -69,6 +70,9 @@ class TransactionView : public QWidget
  70 |      QLineEdit *addressWidget;
  71 |      QLineEdit *amountWidget;
  72 |  
  73 | +    QTimer *m_input_delay_prefix;
  74 | +    QTimer *m_input_delay_amount;

promag commented at 12:15 AM on August 19, 2017:

Same as above.

in src/qt/transactionview.h:115 in 6d073a2d1c outdated

 115 | @@ -112,8 +116,8 @@ public Q_SLOTS:
 116 |      void chooseDate(int idx);
 117 |      void chooseType(int idx);
 118 |      void chooseWatchonly(int idx);
 119 | -    void changedPrefix(const QString &prefix);
 120 | -    void changedAmount(const QString &amount);
 121 | +    void changedPrefix();
 122 | +    void changedAmount();

promag commented at 12:18 AM on August 19, 2017:

Nit, I would sort the slots.

in src/qt/transactionview.cpp:119 in 6d073a2d1c outdated

 112 | @@ -112,6 +113,17 @@ TransactionView::TransactionView(const PlatformStyle *platformStyle, QWidget *pa
 113 |      amountWidget->setValidator(new QDoubleValidator(0, 1e20, 8, this));
 114 |      hlayout->addWidget(amountWidget);
 115 |  
 116 | +    /* Delay before before filtering transactions in ms */
 117 | +    static const int inputFilterDelay = 200;
 118 | +
 119 | +    m_input_delay_amount = new QTimer(this);

promag commented at 12:21 AM on August 19, 2017:

Nit, change to QTimer* amount_typing_delay = new QTimer(this);

in src/qt/transactionview.cpp:116 in 6d073a2d1c outdated

 112 | @@ -112,6 +113,17 @@ TransactionView::TransactionView(const PlatformStyle *platformStyle, QWidget *pa
 113 |      amountWidget->setValidator(new QDoubleValidator(0, 1e20, 8, this));
 114 |      hlayout->addWidget(amountWidget);
 115 |  
 116 | +    /* Delay before before filtering transactions in ms */

promag commented at 12:24 AM on August 19, 2017:

Comments is this file are // ....

promag changes_requested

promag commented at 12:26 AM on August 19, 2017: member

For reference https://wiki.qt.io/Delay_action_to_wait_for_user_interaction.

jonasschnelli commented at 3:50 PM on September 7, 2017: contributor

@lclc, could you address @promag's points? I'd like to get this in soon.

lclc commented at 5:30 AM on September 11, 2017: contributor

I'll do it on Tuesday night.

Am 07.09.2017 17:52 schrieb "Jonas Schnelli" notifications@github.com:

@lclc https://github.com/lclc, could you address @promag https://github.com/promag's points? I'd like to get this in soon.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/11015#issuecomment-327841890, or mute the thread https://github.com/notifications/unsubscribe-auth/ABwrESCkYZHmbXMwSNGUFJuiEzuXFc7Rks5sgBEigaJpZM4OxrEB .

lclc force-pushed on Sep 12, 2017

lclc commented at 5:40 PM on September 12, 2017: contributor

Updated.

Thanks for the reviews @promag - this version is now quite a bit simpler and cleaner by now than the first one :)

promag commented at 7:07 PM on September 12, 2017: member

utACK a82976d. Will test in a couple of hours.

promag commented at 9:59 PM on September 12, 2017: member

Tested ACK a82976d.

flack commented at 7:56 PM on September 15, 2017: contributor

If the motivation is to make filtering large data sets easier, why not apply the delay only when the data set is sufficiently large? For other cases, it just reduces responsiveness. Or is the size of the data set not known when the UI is constructed?

promag commented at 8:23 PM on September 15, 2017: member

200ms is very reasonable. See https://ux.stackexchange.com/a/38545.

jonasschnelli commented at 3:31 AM on September 18, 2017: contributor

Tested ACK a82976d96009680ab61bcea763f15c7594b9afb0

[Qt] Add delay before filtering transactions

Fixes 3141

7b137acedd

in src/qt/transactionview.cpp:116 in a82976d960 outdated

 112 | @@ -112,6 +113,17 @@ TransactionView::TransactionView(const PlatformStyle *platformStyle, QWidget *pa
 113 |      amountWidget->setValidator(new QDoubleValidator(0, 1e20, 8, this));
 114 |      hlayout->addWidget(amountWidget);
 115 |  
 116 | +    // Delay before before filtering transactions in ms

jonasschnelli commented at 3:31 AM on September 18, 2017:

nit: comment (before before)

lclc commented at 8:54 AM on September 18, 2017:

Thanks, fixed.

lclc force-pushed on Sep 18, 2017

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

Tested ACK 7b137ac.

jonasschnelli merged this on Sep 26, 2017

jonasschnelli closed this on Sep 26, 2017

jonasschnelli referenced this in commit 2505c5c0a9 on Sep 26, 2017

MarcoFalke referenced this in commit 6b682f378f on Oct 3, 2017

MarcoFalke referenced this in commit 6642558078 on Oct 3, 2017

jasonbcox referenced this in commit f74e946c56 on Aug 16, 2019

codablock referenced this in commit 61c5fb9ef7 on Sep 25, 2019

jonspock referenced this in commit 09fa1b3900 on Dec 8, 2019

jonspock referenced this in commit 8ed458fbff on Dec 8, 2019

proteanx referenced this in commit 0f9e053728 on Dec 12, 2019

barrystyle referenced this in commit 93bcf814aa on Jan 22, 2020

MarcoFalke locked this on Sep 8, 2021