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

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

Merged
merged 5 commits into from
Jan 12, 2022

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Jan 6, 2022

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

@hebasto
Copy link
Member Author

hebasto commented Jan 6, 2022

cc @shaavan @promag @jarolrod

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 7, 2022

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.

Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 1009baf.

Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review ACK 1009baf

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

src/qt/clientmodel.cpp Outdated Show resolved Hide resolved
src/qt/clientmodel.cpp Show resolved Hide resolved
Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 1009baf

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

in 94f914b, 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

@hebasto
Copy link
Member Author

hebasto commented Jan 9, 2022

Updated 1009baf -> f3bdc14 (pr517.01 -> pr517.02):

@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
Copy link
Member

re-ACK f3bdc14

Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK f3bdc14.

But there are other cases like

timer->setInterval(msecsPerSample);

static const int input_filter_delay = 200;

prefix_typing_delay->setInterval(input_filter_delay);

@hebasto
Copy link
Member Author

hebasto commented Jan 12, 2022

Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 51250b0.

Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reACK 51250b0

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 hebasto merged commit 16781e1 into bitcoin-core:master Jan 12, 2022
@hebasto hebasto deleted the 220106-chrono branch January 12, 2022 13:00
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 12, 2022
…ers of QTimer methods

51250b0 refactor, qt: Use std::chrono for input_filter_delay constant (Hennadii Stepanov)
f3bdc14 refactor, qt: Add SHUTDOWN_POLLING_DELAY constant (Hennadii Stepanov)
0e193de refactor, qt: Use std::chrono for non-zero arguments in QTimer methods (Hennadii Stepanov)
6f0da95 refactor, qt: Use std::chrono in ConfirmMessage parameter (Hennadii Stepanov)
33d520a refactor, qt: Use std::chrono for MODEL_UPDATE_DELAY constant (Hennadii Stepanov)

Pull request description:

  Since Qt 5.8 `QTimer` methods have overloads that accept `std::chrono::milliseconds` arguments:
  - [`QTimer::singleShot`](https://doc.qt.io/archives/qt-5.9/qtimer.html#singleShot-8)
  - [`QTimer::start`](https://doc.qt.io/archives/qt-5.9/qtimer.html#start-2)

ACKs for top commit:
  promag:
    Code review ACK 51250b0.
  shaavan:
    reACK 51250b0

Tree-SHA512: aa843bb2322a84c0c2bb113d3b48d7bf02d7f09a770779dcde312c32887f973ef9445cdef42f39edaa599ff0f3d0457454f6153aa130efadd989e413d39c6062
hebasto added a commit that referenced this pull request Feb 4, 2022
…l() argument

f7a19ef qt,refactor: Use std::chrono in TrafficGraphWidget class (Shashwat)

Pull request description:

  The PR is a follow-up to #517

  - It addresses the change suggested in [this](#517 (review)) comment.
  - This PR changes the type of `msecsPerSample` from **int** to **std::chrono::minutes** and makes other relevant subsequent changes that were limited to the **trafficgraphwidget** file.

ACKs for top commit:
  RandyMcMillan:
    tACK f7a19ef
  hebasto:
    ACK f7a19ef
  promag:
    Code review ACK f7a19ef.

Tree-SHA512: 5094ba894f3051fc99148cb8f408fc6f9d6571188673dcb7bf24366cdfb3eaf6d4e41083685d578ad2a9fbe31cc491a5f3fa9b7c9ab6eb90e4dc1356f89ae18a
@bitcoin-core bitcoin-core locked and limited conversation to collaborators Jan 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants