Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Qt] Add delay before filtering transactions #11015

Merged
merged 1 commit into from Sep 26, 2017

Conversation

Projects
None yet
8 participants
@lclc
Copy link
Contributor

lclc commented Aug 9, 2017

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 fanquake added the GUI label Aug 9, 2017

@promag
Copy link
Member

promag left a comment

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

src/qt/transactionview.cpp Outdated
connect(amountWidget, SIGNAL(textChanged(QString)), this, SLOT(changedAmount(QString)));
connect(addressWidget, SIGNAL(textChanged(QString)), this, SLOT(changedPrefix()));
connect(inputDelayPrefix, SIGNAL(timeout()), this, SLOT(filterByPrefix()));
connect(amountWidget, SIGNAL(textChanged(QString)), this, SLOT(changedAmount()));

This comment has been minimized.

@promag

promag Aug 9, 2017

Member

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

src/qt/transactionview.cpp Outdated
@@ -173,8 +180,10 @@ TransactionView::TransactionView(const PlatformStyle *platformStyle, QWidget *pa
connect(dateWidget, SIGNAL(activated(int)), this, SLOT(chooseDate(int)));
connect(typeWidget, SIGNAL(activated(int)), this, SLOT(chooseType(int)));
connect(watchOnlyWidget, SIGNAL(activated(int)), this, SLOT(chooseWatchonly(int)));
connect(addressWidget, SIGNAL(textChanged(QString)), this, SLOT(changedPrefix(QString)));
connect(amountWidget, SIGNAL(textChanged(QString)), this, SLOT(changedAmount(QString)));
connect(addressWidget, SIGNAL(textChanged(QString)), this, SLOT(changedPrefix()));

This comment has been minimized.

@promag

promag Aug 9, 2017

Member

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

src/qt/transactionview.cpp Outdated
transactionProxyModel->setAddressPrefix(addressWidget->text());
}

void TransactionView::changedAmount()

This comment has been minimized.

@promag

promag Aug 9, 2017

Member

Can be removed.

@promag

This comment has been minimized.

Copy link
Member

promag commented Aug 9, 2017

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

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Aug 9, 2017

Concept ACK

@lclc lclc force-pushed the lclc:searchDelay branch Aug 9, 2017

@lclc lclc changed the title [Qt] Search TX: Add delay before filtering [Qt] Add delay before filtering transactions Aug 9, 2017

@lclc

This comment has been minimized.

Copy link
Contributor Author

lclc commented Aug 9, 2017

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

This comment has been minimized.

Copy link
Member

sipa commented Aug 9, 2017

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

@promag
Copy link
Member

promag left a comment

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

src/qt/transactionview.h Outdated
QTimer *inputDelayAmount;

/* Delay before before filtering transactions in ms */
static const int inputFilterDelay = 200;

This comment has been minimized.

@promag

promag Aug 9, 2017

Member

Move to .cpp?

src/qt/transactionview.h Outdated
@@ -69,6 +70,12 @@ class TransactionView : public QWidget
QLineEdit *addressWidget;
QLineEdit *amountWidget;

QTimer *inputDelayPrefix;

This comment has been minimized.

@promag

promag Aug 9, 2017

Member

The convention is to prefix with m_ in new code.

This comment has been minimized.

@lclc

lclc Aug 9, 2017

Author Contributor

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.

This comment has been minimized.

@promag

promag Aug 9, 2017

Member

@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.

This comment has been minimized.

@lclc

lclc Aug 9, 2017

Author Contributor

Ok, thanks. I'll update.

@lclc lclc force-pushed the lclc:searchDelay branch Aug 9, 2017

@lclc

This comment has been minimized.

Copy link
Contributor Author

lclc commented Aug 9, 2017

Improved as suggested by @promag.

@benma
Copy link
Contributor

benma left a comment

ACK 0587d2e36a0a64535e8d296fb97fcb7a258637fe

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

src/qt/transactionview.h Outdated
@@ -69,6 +70,9 @@ class TransactionView : public QWidget
QLineEdit *addressWidget;
QLineEdit *amountWidget;

QTimer *m_inputDelayPrefix;

This comment has been minimized.

@benma

benma Aug 9, 2017

Contributor

please use snake_case as per developer notes.

This comment has been minimized.

@jonasschnelli

jonasschnelli Aug 10, 2017

Member

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.

This comment has been minimized.

@sipa

sipa Aug 10, 2017

Member

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.

This comment has been minimized.

@jonasschnelli

jonasschnelli Aug 10, 2017

Member

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.

src/qt/transactionview.h Outdated
@@ -112,8 +116,8 @@ public Q_SLOTS:
void chooseDate(int idx);
void chooseType(int idx);
void chooseWatchonly(int idx);
void changedPrefix(const QString &prefix);
void changedAmount(const QString &amount);
void changedPrefix();

This comment has been minimized.

@benma

benma Aug 9, 2017

Contributor

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

This comment has been minimized.

@lclc

lclc Aug 10, 2017

Author Contributor

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.

This comment has been minimized.

@benma

benma Aug 10, 2017

Contributor

+1

This comment has been minimized.

@promag

promag Aug 19, 2017

Member

Agree, all is done in this PR context.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Aug 10, 2017

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 lclc force-pushed the lclc:searchDelay branch Aug 10, 2017

@lclc

This comment has been minimized.

Copy link
Contributor Author

lclc commented Aug 10, 2017

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

@benma

This comment has been minimized.

Copy link
Contributor

benma commented Aug 10, 2017

ACK 6d073a2d1c85ecc523d13c5f47514daa5de4757c

src/qt/transactionview.h Outdated
@@ -69,6 +70,9 @@ class TransactionView : public QWidget
QLineEdit *addressWidget;
QLineEdit *amountWidget;

QTimer *m_input_delay_prefix;

This comment has been minimized.

@promag

promag Aug 18, 2017

Member

There is no need to have these here..

This comment has been minimized.

@promag

promag Aug 19, 2017

Member

Please remove.

@promag

This comment has been minimized.

Copy link
Member

promag commented Aug 18, 2017

utACK 6d073a2.

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Aug 18, 2017

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

This comment has been minimized.

Copy link
Member

promag commented Aug 18, 2017

@jonasschnelli yes start() resets.

@promag
Copy link
Member

promag left a comment

src/qt/transactionview.cpp Outdated
@@ -112,6 +113,17 @@ TransactionView::TransactionView(const PlatformStyle *platformStyle, QWidget *pa
amountWidget->setValidator(new QDoubleValidator(0, 1e20, 8, this));
hlayout->addWidget(amountWidget);

/* Delay before before filtering transactions in ms */
static const int inputFilterDelay = 200;

This comment has been minimized.

@promag

promag Aug 19, 2017

Member

Use snake case. Comment to indicate unit?

FWIW there is this global constant cc @sipa.

src/qt/transactionview.cpp Outdated
{
if(!transactionProxyModel)
return;
CAmount amount_parsed = 0;
if(BitcoinUnits::parse(model->getOptionsModel()->getDisplayUnit(), amount, &amount_parsed))
if(BitcoinUnits::parse(model->getOptionsModel()->getDisplayUnit(), amountWidget->text(), &amount_parsed))

This comment has been minimized.

@promag

promag Aug 19, 2017

Member

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

src/qt/transactionview.h Outdated
@@ -69,6 +70,9 @@ class TransactionView : public QWidget
QLineEdit *addressWidget;
QLineEdit *amountWidget;

QTimer *m_input_delay_prefix;

This comment has been minimized.

@promag

promag Aug 19, 2017

Member

Please remove.

src/qt/transactionview.h Outdated
@@ -69,6 +70,9 @@ class TransactionView : public QWidget
QLineEdit *addressWidget;
QLineEdit *amountWidget;

QTimer *m_input_delay_prefix;
QTimer *m_input_delay_amount;

This comment has been minimized.

@promag

promag Aug 19, 2017

Member

Same as above.

src/qt/transactionview.h Outdated
@@ -112,8 +116,8 @@ public Q_SLOTS:
void chooseDate(int idx);
void chooseType(int idx);
void chooseWatchonly(int idx);
void changedPrefix(const QString &prefix);
void changedAmount(const QString &amount);
void changedPrefix();

This comment has been minimized.

@promag

promag Aug 19, 2017

Member

Agree, all is done in this PR context.

src/qt/transactionview.h Outdated
void changedPrefix(const QString &prefix);
void changedAmount(const QString &amount);
void changedPrefix();
void changedAmount();

This comment has been minimized.

@promag

promag Aug 19, 2017

Member

Nit, I would sort the slots.

src/qt/transactionview.cpp Outdated
/* Delay before before filtering transactions in ms */
static const int inputFilterDelay = 200;

m_input_delay_amount = new QTimer(this);

This comment has been minimized.

@promag

promag Aug 19, 2017

Member

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

src/qt/transactionview.cpp Outdated
@@ -112,6 +113,17 @@ TransactionView::TransactionView(const PlatformStyle *platformStyle, QWidget *pa
amountWidget->setValidator(new QDoubleValidator(0, 1e20, 8, this));
hlayout->addWidget(amountWidget);

/* Delay before before filtering transactions in ms */

This comment has been minimized.

@promag

promag Aug 19, 2017

Member

Comments is this file are // ....

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Sep 7, 2017

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

@lclc

This comment has been minimized.

Copy link
Contributor Author

lclc commented Sep 11, 2017

@lclc lclc force-pushed the lclc:searchDelay branch 3 times, most recently Sep 12, 2017

@lclc

This comment has been minimized.

Copy link
Contributor Author

lclc commented Sep 12, 2017

Updated.

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

@promag

This comment has been minimized.

Copy link
Member

promag commented Sep 12, 2017

utACK a82976d. Will test in a couple of hours.

@promag

This comment has been minimized.

Copy link
Member

promag commented Sep 12, 2017

Tested ACK a82976d.

@flack

This comment has been minimized.

Copy link
Contributor

flack commented Sep 15, 2017

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

This comment has been minimized.

Copy link
Member

promag commented Sep 15, 2017

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

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Sep 18, 2017

Tested ACK a82976d96009680ab61bcea763f15c7594b9afb0

src/qt/transactionview.cpp Outdated
@@ -112,6 +113,17 @@ TransactionView::TransactionView(const PlatformStyle *platformStyle, QWidget *pa
amountWidget->setValidator(new QDoubleValidator(0, 1e20, 8, this));
hlayout->addWidget(amountWidget);

// Delay before before filtering transactions in ms

This comment has been minimized.

@jonasschnelli

jonasschnelli Sep 18, 2017

Member

nit: comment (before before)

This comment has been minimized.

@lclc

lclc Sep 18, 2017

Author Contributor

Thanks, fixed.

@lclc lclc force-pushed the lclc:searchDelay branch to 7b137ac Sep 18, 2017

@promag

This comment has been minimized.

Copy link
Member

promag commented Sep 26, 2017

Tested ACK 7b137ac.

@jonasschnelli jonasschnelli merged commit 7b137ac into bitcoin:master Sep 26, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

jonasschnelli added a commit that referenced this pull request Sep 26, 2017

Merge #11015: [Qt] Add delay before filtering transactions
7b137ac [Qt] Add delay before filtering transactions Fixes 3141 (Lucas Betschart)

Pull request description:

  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.

Tree-SHA512: ee599367794eac2c5b8bc7ecac47f44295e40c0ff543ff2f2c4860590f917b59b1cfb273fa564e6eb4c44016c0ef412d49f1a8f1b36b07e034022f51bb76653c

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Oct 3, 2017

[Qt] Add delay before filtering transactions
Fixes 3141

Github-Pull: bitcoin#11015
Rebased-From: 7b137ac
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.