[Qt] Add delay before filtering transactions by lclc · Pull Request #11015 · bitcoin/bitcoin

lclc · 2017-08-09T07:00:29Z

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.

promag

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

promag · 2017-08-09T07:12:50Z

src/qt/transactionview.cpp

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

promag · 2017-08-09T07:13:19Z

src/qt/transactionview.cpp

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

promag · 2017-08-09T07:15:47Z

src/qt/transactionview.cpp

Can be removed.

promag · 2017-08-09T07:18:44Z

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

jonasschnelli · 2017-08-09T07:30:56Z

Concept ACK

lclc · 2017-08-09T09:00:47Z

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 · 2017-08-09T09:02:00Z

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

promag

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

promag · 2017-08-09T09:56:21Z

src/qt/transactionview.h

Move to .cpp?

promag · 2017-08-09T09:56:38Z

src/qt/transactionview.h

The convention is to prefix with m_ in new code.

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.

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

Ok, thanks. I'll update.

lclc · 2017-08-09T19:08:38Z

Improved as suggested by @promag.

benma

ACK 0587d2e36a0a64535e8d296fb97fcb7a258637fe

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

benma · 2017-08-09T21:06:08Z

src/qt/transactionview.h

please use snake_case as per developer notes.

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.

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.

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.

benma · 2017-08-09T21:10:40Z

src/qt/transactionview.h

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

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.

Agree, all is done in this PR context.

laanwj · 2017-08-10T17:59:18Z

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 · 2017-08-10T19:01:30Z

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

benma · 2017-08-10T19:35:48Z

ACK 6d073a2d1c85ecc523d13c5f47514daa5de4757c

promag · 2017-08-18T07:41:14Z

src/qt/transactionview.h

There is no need to have these here..

Please remove.

promag · 2017-08-18T07:42:07Z

utACK 6d073a2.

jonasschnelli · 2017-08-18T10:02:30Z

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 · 2017-08-18T10:26:40Z

@jonasschnelli yes start() resets.

promag

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

promag · 2017-08-19T00:14:33Z

src/qt/transactionview.cpp

Use snake case. Comment to indicate unit?

FWIW there is this global constant cc @sipa.

promag · 2017-08-19T00:15:14Z

src/qt/transactionview.cpp

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

promag · 2017-08-19T00:15:52Z

src/qt/transactionview.h

Please remove.

promag · 2017-08-19T00:15:59Z

src/qt/transactionview.h

Same as above.

promag · 2017-08-19T00:17:57Z

src/qt/transactionview.h

Agree, all is done in this PR context.

promag · 2017-08-19T00:18:21Z

src/qt/transactionview.h

Nit, I would sort the slots.

promag · 2017-08-19T00:21:40Z

src/qt/transactionview.cpp

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

promag · 2017-08-19T00:24:50Z

src/qt/transactionview.cpp

Comments is this file are // ....

jonasschnelli · 2017-09-07T15:50:45Z

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

lclc · 2017-09-11T05:30:27Z

I'll do it on Tuesday night. Am 07.09.2017 17:52 schrieb "Jonas Schnelli" <[email protected]>:

…

@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 <#11015 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABwrESCkYZHmbXMwSNGUFJuiEzuXFc7Rks5sgBEigaJpZM4OxrEB> .

lclc · 2017-09-12T17:40:12Z

Updated.

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

promag · 2017-09-12T19:07:12Z

utACK a82976d. Will test in a couple of hours.

promag · 2017-09-12T21:59:54Z

Tested ACK a82976d.

flack · 2017-09-15T19:56:20Z

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 · 2017-09-15T20:23:01Z

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

jonasschnelli · 2017-09-18T03:31:15Z

Tested ACK a82976d96009680ab61bcea763f15c7594b9afb0

jonasschnelli · 2017-09-18T03:31:44Z

src/qt/transactionview.cpp

nit: comment (before before)

Thanks, fixed.

Fixes 3141

promag · 2017-09-26T12:32:35Z

Tested ACK 7b137ac.

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

Fixes 3141 Github-Pull: bitcoin#11015 Rebased-From: 7b137ac

Summary: 7b137ac [Qt] Add delay before filtering transactions Fixes 3141 (Lucas Betschart) Pull request description: As discussed in bitcoin/bitcoin#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 Backport of Core PR11015 bitcoin/bitcoin#11015 Test Plan: make check ./bitcoin-qt -> transactions -> enter a filter Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3832

7b137ac [Qt] Add delay before filtering transactions Fixes 3141 (Lucas Betschart) Pull request description: As discussed in bitcoin#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

Summary: 7b137aced [Qt] Add delay before filtering transactions Fixes 3141 (Lucas Betschart) Pull request description: As discussed in bitcoin/bitcoin#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 Backport of Core PR11015 bitcoin/bitcoin#11015 Test Plan: make check ./bitcoin-qt -> transactions -> enter a filter Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3832

fanquake added the GUI label Aug 9, 2017

promag reviewed Aug 9, 2017

View reviewed changes

lclc force-pushed the searchDelay branch from 9943f49 to 1a03f63 Compare August 9, 2017 08:56

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

promag reviewed Aug 9, 2017

View reviewed changes

lclc force-pushed the searchDelay branch from 1a03f63 to 0587d2e Compare August 9, 2017 17:13

benma reviewed Aug 9, 2017

View reviewed changes

lclc force-pushed the searchDelay branch from 0587d2e to 6d073a2 Compare August 10, 2017 18:51

promag reviewed Aug 18, 2017

View reviewed changes

promag suggested changes Aug 19, 2017

View reviewed changes

lclc force-pushed the searchDelay branch 3 times, most recently from 47145f6 to a82976d Compare September 12, 2017 17:22

jonasschnelli reviewed Sep 18, 2017

View reviewed changes

[Qt] Add delay before filtering transactions

7b137ac

Fixes 3141

lclc force-pushed the searchDelay branch from a82976d to 7b137ac Compare September 18, 2017 08:54

promag mentioned this pull request Sep 26, 2017

Qt: Enable searching by transaction id #11395

Merged

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

jonasschnelli mentioned this pull request Sep 26, 2017

Automatic filter should be delayed until input is paused #3141

Closed

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Oct 3, 2017

[Qt] Add delay before filtering transactions

6642558

Fixes 3141 Github-Pull: bitcoin#11015 Rebased-From: 7b137ac

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

lclc commented Aug 9, 2017

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Aug 9, 2017

Uh oh!

jonasschnelli commented Aug 9, 2017

Uh oh!

lclc commented Aug 9, 2017

Uh oh!

sipa commented Aug 9, 2017

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lclc commented Aug 9, 2017

Uh oh!

benma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa Aug 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Aug 10, 2017

Uh oh!

lclc commented Aug 10, 2017

Uh oh!

benma commented Aug 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Aug 18, 2017

Uh oh!

jonasschnelli commented Aug 18, 2017

Uh oh!

promag commented Aug 18, 2017

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

sipa Aug 10, 2017 •

edited

Loading