refactor, qt: Use std::chrono for parameters of QTimer methods#517
refactor, qt: Use std::chrono for parameters of QTimer methods#517hebasto merged 5 commits intobitcoin-core:masterfrom
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
shaavan
left a comment
There was a problem hiding this comment.
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.
jarolrod
left a comment
There was a problem hiding this comment.
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
A named constant is better for the code readability. Also it could be reused in an alternative GUI implementation (e.g., QML-based).
|
Updated 1009baf -> f3bdc14 (pr517.01 -> pr517.02):
Don't want to choose units for arguments :) |
|
re-ACK f3bdc14 |
There was a problem hiding this comment.
Code review ACK f3bdc14.
But there are other cases like
gui/src/qt/trafficgraphwidget.cpp
Line 161 in 1f7acfd
gui/src/qt/transactionview.cpp
Line 118 in 1f7acfd
gui/src/qt/transactionview.cpp
Line 126 in 1f7acfd
|
Thanks @promag
Added a commit which addresses the case in Refactoring of |
shaavan
left a comment
There was a problem hiding this comment.
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_delayhas 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.
…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
…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
Since Qt 5.8
QTimermethods have overloads that acceptstd::chrono::millisecondsarguments:QTimer::singleShotQTimer::start