23@@ -22,7 +24,7 @@ class TrafficGraphWidget : public QWidget
24 public:
25 explicit TrafficGraphWidget(QWidget *parent = nullptr);
26 void setClientModel(ClientModel *model);
27- int getGraphRangeMins() const;
28+ std::chrono::minutes getGraphRangeMins() const;
0 std::chrono::minutes getGraphRange() const;
Also s/setGraphRangeMins/setGraphRange/
38@@ -37,7 +39,7 @@ public Q_SLOTS:
39
40 QTimer *timer;
41 float fMax;
42- int nMins;
43+ std::chrono::minutes nMins;
0 std::chrono::minutes m_range;
24 TrafficGraphWidget::TrafficGraphWidget(QWidget *parent) :
25 QWidget(parent),
26 timer(nullptr),
27 fMax(0.0f),
28- nMins(0),
29+ nMins(0min),
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, 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.
1139@@ -1140,7 +1140,7 @@ void RPCConsole::on_sldGraphRange_valueChanged(int value)
1140
1141 void RPCConsole::setTrafficGraphRange(int mins)
1142 {
1143- ui->trafficGraph->setGraphRangeMins(mins);
1144+ ui->trafficGraph->setGraphRange(std::chrono::minutes(mins));
0 ui->trafficGraph->setGraphRange(std::chrono::minutes{mins});
Should the include header be added? e.g. include what you use.
0 #include <QTimer>
1 #include <QVariant>
2
3+#include <chrono>
4+
5 const int CONSOLE_HISTORY = 50;
159+void TrafficGraphWidget::setGraphRange(std::chrono::minutes mins)
160 {
161- nMins = mins;
162- int msecsPerSample = nMins * 60 * 1000 / DESIRED_SAMPLES;
163+ m_range = std::chrono::minutes(mins);
164+ auto msecsPerSample = m_range * 60 * 1000 / DESIRED_SAMPLES;
0- m_range = std::chrono::minutes(mins);
1- auto msecsPerSample = m_range * 60 * 1000 / DESIRED_SAMPLES;
2+ m_range = std::chrono::minutes{mins};
3+ auto msecsPerSample{m_range * 60 * 1000 / DESIRED_SAMPLES};
43@@ -42,9 +44,9 @@ void TrafficGraphWidget::setClientModel(ClientModel *model)
44 }
45 }
46
47-int TrafficGraphWidget::getGraphRangeMins() const
48+std::chrono::minutes TrafficGraphWidget::getGraphRange() const
49 {
50- return nMins;
51+ return m_range;
nit, the getter could be just one line, feel free to ignore
0std::chrono::minutes TrafficGraphWidget::getGraphRange() const { return m_range; }
I think it’s better to incorporate the correct way of writing the code from the get-go.
Addressed in 5e6ad43
Updated from 9bb48bf to 5e6ad43 (pr524.02 -> pr524.03) Addressed @jonatack and @hebasto’s suggestions.
duration_casting instead of static_casting, as suggested by @hebasto. The reason for doing so is explained in this article.Tested that the network traffic graph is working correctly now.
18
19 #define XMARGIN 10
20 #define YMARGIN 10
21
22+using namespace std::chrono_literals;
23+
I checked through the file and made sure that this statement is not needed.
Removed in f7a19ef774ef92ce348215593e3590a750c345e1
151@@ -153,12 +152,12 @@ void TrafficGraphWidget::updateRates()
152 update();
153 }
154
155-void TrafficGraphWidget::setGraphRangeMins(int mins)
156+void TrafficGraphWidget::setGraphRange(std::chrono::minutes mins)
157 {
158- nMins = mins;
159- int msecsPerSample = nMins * 60 * 1000 / DESIRED_SAMPLES;
160+ m_range = std::chrono::minutes{mins};
Maybe just
0 m_range = mins;
?
The type-conversion here seems redundant.
Addressed in f7a19ef774ef92ce348215593e3590a750c345e1
Approach ACK 5e6ad435b0ed634fca5745668bf949799b306bff, tested on Linux Mint 20.3 (Qt 5.12.8).
About commit message:
PR#517 do not work well with separated repositories that use the monotree; you could use bitcoin-core/gui#517, or just drop it31 void paintEvent(QPaintEvent *) override;
32
33 public Q_SLOTS:
34 void updateRates();
35- void setGraphRangeMins(int mins);
36+ void setGraphRange(std::chrono::minutes mins);
Maybe
0 void setGraphRange(std::chrono::minutes new_range);
?
I think new_range is a more suitable name for the argument, and now its unit is cleared by its type (i.e., minutes)
Addressed in f7a19ef774ef92ce348215593e3590a750c345e1
Updated from 5e6ad43 to f7a19ef (pr524.03 -> pr524.04) Addressed @hebasto suggestions
Changes:
using namespace std::chrono_literals; statement.chrono::minutes variable.mins -> new_range.
tACK f7a19ef774ef92ce348215593e3590a750c345e1
on MacOS x86_64 and Arm64
23@@ -22,22 +24,22 @@ class TrafficGraphWidget : public QWidget
24 public:
25 explicit TrafficGraphWidget(QWidget *parent = nullptr);
26 void setClientModel(ClientModel *model);
27- int getGraphRangeMins() const;
28+ std::chrono::minutes getGraphRange() const;
I concur - It appears to be part of the original implementation but never used for anything…
it can and should be removed…
147@@ -153,12 +148,12 @@ void TrafficGraphWidget::updateRates()
148 update();
149 }
150
151-void TrafficGraphWidget::setGraphRangeMins(int mins)
152+void TrafficGraphWidget::setGraphRange(std::chrono::minutes new_range)
m_range and rename this arg to range.
if (m_range == new_range) return, but I think this won’t happen in practice.
Drop
m_rangeand rename this arg torange.
In a separate (first?) commit, no?
nMin instead of rename.
TrafficGraphWidget::getGraphRangeMins() and TrafficGraphWidget::nMin, and (b) refactoring of TrafficGraphWidget::setGraphRangeMins() (renaming and parameter type changing) looks really as two focused commits, no?
I think this is a valid suggestion. Better to get rid of a redundant member variable.
Addressed in bdf7cb9f0c17bdeeb8389d4b9140aca65e08ac71 @hebasto
(a) getting rid of TrafficGraphWidget::getGraphRangeMins() and TrafficGraphWidget::nMin, and (b) refactoring of TrafficGraphWidget::setGraphRangeMins() (renaming and parameter type changing) looks really as two focused commits, no?
I agree with your reasoning. However, (a) and (b) are not mutually exclusive, and hence I was having a hard time splitting these two changes while keeping the commits meaningful. Therefore, I reckoned it would be best to make these changes in one commit.
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 https://github.com/bitcoin-core/gui/pull/539/commits/c25d3f42084926c92ae2051278cd785cb1907d56 to ensure compatibility with the chrono lib.
I use int TrafficGraphWidget::getGraphRangeMins() const in PR #539 (which fixes issue #532)
I looked through #532, but I couldn’t find any use case of getGraphRangeMins() there. So I don’t think there should be an issue removing this function in this PR.
I have added a variation of this PR to #539 c25d3f4 to ensure compatibility with the chrono lib.
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.
It will have to be re-added - it is easier to leave it in.
I think you are right. Let me keep the GetGraphRange() function and make it consistent with the previous changes.
Updated from bdf7cb9 to cad0e0a (pr524.06 -> pr524.07, diff) Addressed @RandyMcMillan comment
Changes:
getGraphRange() function, as it is used in #539. So removing and re-adding the function doesn’t seem optimal.The updated PR correctly compiles on Ubuntu 20.04, with the Network Traffic widget showing the correct graph.
Reverted back to f7a19ef774ef92ce348215593e3590a750c345e1 (pr524.07 -> pr524.04, diff)
Reason:
P.S.: To avoid any unintended changes between the PR and f7a19ef774ef92ce348215593e3590a750c345e1, I used the following command to update the PR:
0git reset --hard HEAD^
1git cherry-pick f7a19ef774ef92ce348215593e3590a750c345e1
The commit id has changed (which I was not able to restore) but the changes are identical f7a19ef774ef92ce348215593e3590a750c345e1
Restored PR to previous commit f7a19ef774ef92ce348215593e3590a750c345e1. The reason for doing so is explained here: #524 (comment)
P.s.: Thanks, @hebasto, for your help in restoring this commit.
