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
Replace int with std::chrono in for the timer->setInterval() argument #524
Conversation
6e947e6
to
9bb48bf
Compare
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested 9bb48bf on Linux Mint 20.3 (Qt 5.12.8) -- the "Network Traffic" tab shows a black window.
9bb48bf
to
5e6ad43
Compare
Updated from 9bb48bf to 5e6ad43 (pr524.02 -> pr524.03)
Tested that the network traffic graph is working correctly now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach ACK 5e6ad43, tested on Linux Mint 20.3 (Qt 5.12.8).
About commit message:
- please use "qt: " prefix (see https://github.com/bitcoin-core/gui/blob/master/CONTRIBUTING.md#creating-the-pull-request)
- what about "qt, refactor: Use std::chrono in TrafficGraphWidget class"?
- references like
PR#517
do not work well with separated repositories that use the monotree; you could usebitcoin-core/gui#517
, or just drop it
5e6ad43
to
f7a19ef
Compare
Updated from 5e6ad43 to f7a19ef (pr524.03 -> pr524.04) Changes:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK f7a19ef
tACK f7a19ef on MacOS x86_64 and Arm64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK f7a19ef.
@@ -22,22 +24,22 @@ class TrafficGraphWidget : public QWidget | |||
public: | |||
explicit TrafficGraphWidget(QWidget *parent = nullptr); | |||
void setClientModel(ClientModel *model); | |||
int getGraphRangeMins() const; | |||
std::chrono::minutes getGraphRange() const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just remove? It's not used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I concur - It appears to be part of the original implementation but never used for anything...
it can and should be removed...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@promag - great catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach ACK Removing the TrafficGraphWidget::getGraphRangeMins() function nACK
I use int TrafficGraphWidget::getGraphRangeMins() const in PR #539 (which fixes issue #532)
I have added a variation to of this PR to #539 c25d3f4 to ensure compatibility with the chrono lib.
gui/src/qt/trafficgraphwidget.cpp
Line 46 in c25d3f4
Note
f7a19ef
to
5efd391
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK 5efd391 on Ubuntu 21.10 (Qt 5.15.2).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-ACK 5efd391
5efd391
to
bdf7cb9
Compare
Approach ACK I use int TrafficGraphWidget::getGraphRangeMins() const in PR #539 (which fixes issue #532) I have added a variation to of this PR to #539 c25d3f4 to ensure compatibility with the chrono lib. gui/src/qt/trafficgraphwidget.cpp Line 46 in c25d3f4 Note |
I looked through #532, but I couldn't find any use case of
I looked through it, and it looked good. However, I reckon the primary purpose of #539 is to fix an issue by adding a scaling feature. And the purpose of this PR is refactoring. So I think it would be best to keep these PRs separate. |
If you insist - but to scale the graph - we will have to know the current value of the slider - otherwise the graph can't calculate how many samples to display. It will have to be re-added - it is easier to leave it in. |
I think you are right. Let me keep the |
bdf7cb9
to
cad0e0a
Compare
Updated from bdf7cb9 to cad0e0a (pr524.06 -> pr524.07, diff) Changes:
The updated PR correctly compiles on Ubuntu 20.04, with the Network Traffic widget showing the correct graph. |
cad0e0a
to
88c5b10
Compare
Reverted back to f7a19ef (pr524.07 -> pr524.04, diff) Reason:
P.S.: To avoid any unintended changes between the PR and f7a19ef, I used the following command to update the PR:
The commit id has changed (which I was not able to restore) but the changes are identical f7a19ef |
88c5b10
to
f7a19ef
Compare
Restored PR to previous commit f7a19ef. The reason for doing so is explained here: #524 (comment) P.s.: Thanks, @hebasto, for your help in restoring this commit. |
The PR is a follow-up to #517
msecsPerSample
from int to std::chrono::minutes and makes other relevant subsequent changes that were limited to the trafficgraphwidget file.