refactor, qt: Use std::chrono for parameters of QTimer methods

hebasto commented at 4:48 pm on January 6, 2022: member

Since Qt 5.8 QTimer methods have overloads that accept std::chrono::milliseconds arguments:

hebasto commented at 4:48 pm on January 6, 2022: member

cc @shaavan @promag @jarolrod

hebasto added the label Refactoring on Jan 6, 2022

DrahtBot commented at 8:28 am on January 7, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#497 (Enable users to configure their monospace font specifically by luke-jr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

promag commented at 10:26 am on January 7, 2022: contributor

Code review ACK 1009baf2fc5f67d1761ca8474e33198086f82981.

in src/qt/clientmodel.cpp:292 in 1009baf2fc outdated

288@@ -288,7 +289,7 @@ static void BlockTipChanged(ClientModel* clientmodel, SynchronizationState sync_
289     const bool throttle = (sync_state != SynchronizationState::POST_INIT && !fHeader) || sync_state == SynchronizationState::INIT_REINDEX;
290     const int64_t now = throttle ? GetTimeMillis() : 0;
291     int64_t& nLastUpdateNotification = fHeader ? nLastHeaderTipUpdateNotification : nLastBlockTipUpdateNotification;
292-    if (throttle && now < nLastUpdateNotification + MODEL_UPDATE_DELAY) {
293+    if (throttle && now < nLastUpdateNotification + MODEL_UPDATE_DELAY.count()) {

shaavan commented at 1:01 pm on January 7, 2022:

nit:

0    if (throttle && now < nLastUpdateNotification + count_milliseconds(MODEL_UPDATE_DELAY)) {

We could clarify the unit.

jarolrod commented at 11:58 pm on January 8, 2022:

I think this is a fine clarification if others think it makes this code more readable, additionally there is this line of guidance:

https://github.com/bitcoin-core/gui/blob/6182e5086fc82b49a5f0d2abdab4bc7e2a4686ef/src/util/time.h#L22-L24

hebasto commented at 4:01 pm on January 9, 2022:

Thanks! Updated.

in src/qt/clientmodel.cpp:290 in 1009baf2fc outdated

288@@ -288,7 +289,7 @@ static void BlockTipChanged(ClientModel* clientmodel, SynchronizationState sync_
289     const bool throttle = (sync_state != SynchronizationState::POST_INIT && !fHeader) || sync_state == SynchronizationState::INIT_REINDEX;
290     const int64_t now = throttle ? GetTimeMillis() : 0;

shaavan commented at 1:07 pm on January 7, 2022:

Could we do the following change?

0    const auto now = throttle ? GetTime<std::chrono::milliseconds>() : 0ms;

jarolrod commented at 11:59 pm on January 8, 2022:

We don’t want to do this because we want sys time, not mockable time.

shaavan commented at 6:05 am on January 9, 2022:

we want sys time, not mockable time

That makes sense. Thanks for the clarification.

shaavan approved

shaavan commented at 1:17 pm on January 7, 2022: contributor

Code Review ACK 1009baf2fc5f67d1761ca8474e33198086f82981

I prefer using std::chrono over int64_t for time variables wherever possible. So I agree with the PR’s concept.
Also, the refactor looks clean, and makes code more readable.

I have added some suggestions that might help make this PR even better.

p.s. While reviewing this PR, I came across two functions in the codebase GetTime and GetSystemTime. So I was curious whether these have different functionalities or are similar in their use case.

jarolrod commented at 11:55 pm on January 8, 2022: member

ACK 1009baf2fc5f67d1761ca8474e33198086f82981

Code Review and tested ACK. I’ve verified that all instances of ConfirmMessage have been updated.

in 94f914bbb9a46d5d595bf5fc6575719e48deac10, since the commit message states Use std::chrono in parameters of QTimer methods you could update all instances to use the std::chrono overload, even the 0 ones. Otherwise, this commit is reallly just Use std::chrono in parameters of QTimer methods time is > 0

refactor, qt: Use std::chrono for MODEL_UPDATE_DELAY constant 33d520ac53

refactor, qt: Use std::chrono in ConfirmMessage parameter 6f0da95811

refactor, qt: Use std::chrono for non-zero arguments in QTimer methods 0e193deb52

refactor, qt: Add SHUTDOWN_POLLING_DELAY constant

A named constant is better for the code readability. Also it could be
reused in an alternative GUI implementation (e.g., QML-based).

f3bdc143b6

hebasto force-pushed on Jan 9, 2022

hebasto commented at 4:01 pm on January 9, 2022: member

Updated 1009baf2fc5f67d1761ca8474e33198086f82981 -> f3bdc143b67e8a5e763071a0774f6d994ca35c57 (pr517.01 -> pr517.02):

rebased due to the conflict with #441
addressed @shaavan’s comment
addressed @jarolrod’s comment @jarolrod

you could update all instances to use the std::chrono overload, even the 0 ones.

Don’t want to choose units for arguments :)

jarolrod commented at 6:40 pm on January 10, 2022: member

re-ACK f3bdc14

promag commented at 10:12 am on January 12, 2022: contributor

Code review ACK f3bdc143b67e8a5e763071a0774f6d994ca35c57.

But there are other cases like https://github.com/bitcoin-core/gui/blob/1f7acfdcca3525edf3eba23535240d23ff0c4fc9/src/qt/trafficgraphwidget.cpp#L161 https://github.com/bitcoin-core/gui/blob/1f7acfdcca3525edf3eba23535240d23ff0c4fc9/src/qt/transactionview.cpp#L118 https://github.com/bitcoin-core/gui/blob/1f7acfdcca3525edf3eba23535240d23ff0c4fc9/src/qt/transactionview.cpp#L126

refactor, qt: Use std::chrono for input_filter_delay constant 51250b0906

hebasto commented at 10:27 am on January 12, 2022: member

Thanks @promag

But there are other cases like https://github.com/bitcoin/bitcoin/blob/1f7acfdcca3525edf3eba23535240d23ff0c4fc9/src/qt/trafficgraphwidget.cpp#L161 https://github.com/bitcoin/bitcoin/blob/1f7acfdcca3525edf3eba23535240d23ff0c4fc9/src/qt/transactionview.cpp#L118 https://github.com/bitcoin/bitcoin/blob/1f7acfdcca3525edf3eba23535240d23ff0c4fc9/src/qt/transactionview.cpp#L126

Added a commit which addresses the case in TransactionView::TransactionView.

Refactoring of TrafficGraphWidget I leaved for an follow up, as it looks non-trivial.

promag commented at 11:36 am on January 12, 2022: contributor

Code review ACK 51250b0906e56b39488304208ad119c951b4ae7d.

shaavan approved

shaavan commented at 12:51 pm on January 12, 2022: contributor

reACK 51250b0906e56b39488304208ad119c951b4ae7d

Changes since my last review:

MODEL_UPDATE_DELAY.count() -> count_milliseconds(MODEL_UPDATE_DELAY)
Consequently #include <chrono> -> #include <util/time.h> in src/qt/clientmodel.h
Variable input_filter_delay has been converted from int to std::chrono.

The update takes care of the input_filter_delay. And as for the TrafficGraphWidget variable, let me try giving it a shot.

hebasto merged this on Jan 12, 2022

hebasto closed this on Jan 12, 2022

hebasto deleted the branch on Jan 12, 2022

sidhujag referenced this in commit 802e4142b5 on Jan 12, 2022

hebasto referenced this in commit 5c6b3d5b35 on Feb 4, 2022

bitcoin-core locked this on Jan 12, 2023

refactor, qt: Use std::chrono for parameters of QTimer methods #517

Conflicts