Replace int with std::chrono in for the timer->setInterval() argument by shaavan · Pull Request #524 · bitcoin-core/gui

shaavan · 2022-01-12T13:48:55Z

The PR is a follow-up to #517

It addresses the change suggested in this 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.

src/qt/trafficgraphwidget.h

src/qt/trafficgraphwidget.cpp

shaavan · 2022-01-13T11:37:57Z

Updated from 6e947e6 to 9bb48bf (pr524.01 -> pr524.02)
Addressed @hebasto comments

Changes:

Changed argument type of setGraphRangeMins from int to std::minutes.
setGraphRangeMins -> setGraphRange; getGraphRangeMins -> getGraphRange
Moved nMins to member initialization.
Renamed nMins -> m_range.

DrahtBot · 2022-01-13T14:38:44Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#539 (gui: scale network graph based on time interval by RandyMcMillan)
#492 (Show ToolTip on Network Traffic graph by rebroad)
#484 (Don't clear traffic graph when changing interval by rebroad)

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.

jonatack

Concept ACK

src/qt/rpcconsole.cpp

src/qt/trafficgraphwidget.cpp

hebasto

Tested 9bb48bf on Linux Mint 20.3 (Qt 5.12.8) -- the "Network Traffic" tab shows a black window.

src/qt/trafficgraphwidget.cpp

shaavan · 2022-01-16T07:08:39Z

Updated from 9bb48bf to 5e6ad43 (pr524.02 -> pr524.03)
Addressed @jonatack and @hebasto’s suggestions.

Done proper casting of m_range variable from mins to milliseconds.
Used duration_casting instead of static_casting, as suggested by @hebasto. The reason for doing so is explained in this article.
Incorporated other nit suggestions.

Tested that the network traffic graph is working correctly now.

hebasto

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 use bitcoin-core/gui#517, or just drop it

src/qt/trafficgraphwidget.cpp

src/qt/trafficgraphwidget.h

shaavan · 2022-01-18T14:48:52Z

Updated from 5e6ad43 to f7a19ef (pr524.03 -> pr524.04)
Addressed @hebasto suggestions

Changes:

Removed the redundant using namespace std::chrono_literals; statement.
Removed unnecessary type conversion of chrono::minutes variable.
Renamed mins -> new_range.
Made appropriate changes to commit message as suggested here.

hebasto

ACK f7a19ef

RandyMcMillan · 2022-01-18T21:51:16Z

tACK f7a19ef

on MacOS x86_64 and Arm64

promag

Code review ACK f7a19ef.

promag · 2022-01-20T15:56:22Z

src/qt/trafficgraphwidget.h

    explicit TrafficGraphWidget(QWidget *parent = nullptr);
    void setClientModel(ClientModel *model);
-    int getGraphRangeMins() const;
+    std::chrono::minutes getGraphRange() const;


Just remove? It's not used.

@promag @shaavan

I concur - It appears to be part of the original implementation but never used for anything...

it can and should be removed...

https://github.com/bitcoin-core/gui/blame/16781e1bc9f8ffc721ebea73434e0066957bc959/src/qt/trafficgraphwidget.cpp#L45

@promag - great catch!

Thanks @promag for catching this. Addressed in 5efd391

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

https://github.com/bitcoin-core/gui/pull/524/files#diff-da38549cd85e0c422891b8ef1eb3cc33e76f0b80d15ca63aa7ad6f065c8ba694L44

shaavan · 2022-01-21T12:30:33Z

Updated from f7a19ef to 5efd391 (pr524.04 -> pr524.05)
Addressed @promag suggestions

Changes:

Removed the unused function getGraphRange() from the TrafficGraphWidget class.

w0xlt

tACK 5efd391 on Ubuntu 21.10 (Qt 5.15.2).

hebasto

re-ACK 5efd391

src/qt/trafficgraphwidget.cpp

shaavan · 2022-01-27T14:15:36Z

Updated from 5efd391 to bdf7cb9 (pr524.05 -> pr524.06)
Addressed @promag suggestion

Changes:

Removed m_range as a member variable of the class TrafficGraphWidget and replaced it with a local range variable.

RandyMcMillan · 2022-01-28T16:57:20Z

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

https://github.com/bitcoin-core/gui/pull/524/files#diff-da38549cd85e0c422891b8ef1eb3cc33e76f0b80d15ca63aa7ad6f065c8ba694L44

shaavan · 2022-01-31T10:49:54Z

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.

RandyMcMillan · 2022-02-01T00:02:58Z

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.

#539

shaavan · 2022-02-01T07:30:59Z

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.

shaavan · 2022-02-03T10:11:35Z

Updated from bdf7cb9 to cad0e0a (pr524.06 -> pr524.07, diff)
Addressed @RandyMcMillan comment

Changes:

Restored the getGraphRange() function, as it is used in Scale network graph based on time interval #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.

shaavan · 2022-02-03T11:41:20Z

Reverted back to f7a19ef (pr524.07 -> pr524.04, diff)

Reason:

The is almost no difference between pr524.07 and pr524.04 (see the diff).
f7a19ef had quite a few ACKs behind it, by:

P.S.: To avoid any unintended changes between the PR and f7a19ef, I used the following command to update the PR:

git reset --hard HEAD^
git cherry-pick f7a19ef774ef92ce348215593e3590a750c345e1

The commit id has changed (which I was not able to restore) but the changes are identical f7a19ef

shaavan · 2022-02-04T10:11:27Z

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.

RandyMcMillan · 2022-02-08T18:33:50Z

@shaavan - thanks for leaving that function in - @rebroad and I have work that utilizes this.

hebasto reviewed Jan 12, 2022

View reviewed changes

src/qt/trafficgraphwidget.h Outdated Show resolved Hide resolved

src/qt/trafficgraphwidget.h Outdated Show resolved Hide resolved

src/qt/trafficgraphwidget.cpp Outdated Show resolved Hide resolved

shaavan force-pushed the 220112-chrono branch from 6e947e6 to 9bb48bf Compare January 13, 2022 11:35

DrahtBot mentioned this pull request Jan 13, 2022

Show ToolTip on Network Traffic graph #492

Closed

3 tasks

jonatack reviewed Jan 13, 2022

View reviewed changes

src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved

src/qt/trafficgraphwidget.cpp Outdated Show resolved Hide resolved

src/qt/trafficgraphwidget.cpp Outdated Show resolved Hide resolved

hebasto added the Refactoring label Jan 15, 2022

hebasto reviewed Jan 15, 2022

View reviewed changes

src/qt/trafficgraphwidget.cpp Outdated Show resolved Hide resolved

shaavan force-pushed the 220112-chrono branch from 9bb48bf to 5e6ad43 Compare January 16, 2022 07:06

hebasto reviewed Jan 16, 2022

View reviewed changes

src/qt/trafficgraphwidget.cpp Outdated Show resolved Hide resolved

src/qt/trafficgraphwidget.cpp Outdated Show resolved Hide resolved

hebasto reviewed Jan 16, 2022

View reviewed changes

src/qt/trafficgraphwidget.h Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Jan 16, 2022

Don't clear traffic graph when changing interval #484

Closed

qt,refactor: Use std::chrono in TrafficGraphWidget class

f7a19ef

shaavan force-pushed the 220112-chrono branch from 5e6ad43 to f7a19ef Compare January 18, 2022 14:46

hebasto approved these changes Jan 18, 2022

View reviewed changes

Gemk83 approved these changes Jan 18, 2022

View reviewed changes

promag reviewed Jan 20, 2022

View reviewed changes

shaavan force-pushed the 220112-chrono branch from f7a19ef to 5efd391 Compare January 21, 2022 12:29

DrahtBot mentioned this pull request Jan 21, 2022

Enable a non-linear network traffic option (click to toggle between linear and non-linear) #473

Closed

w0xlt approved these changes Jan 23, 2022

View reviewed changes

hebasto approved these changes Jan 23, 2022

View reviewed changes

promag reviewed Jan 23, 2022

View reviewed changes

src/qt/trafficgraphwidget.cpp Show resolved Hide resolved

shaavan force-pushed the 220112-chrono branch from 5efd391 to bdf7cb9 Compare January 27, 2022 14:14

DrahtBot mentioned this pull request Jan 27, 2022

Scale network graph based on time interval #539

Closed

shaavan force-pushed the 220112-chrono branch from bdf7cb9 to cad0e0a Compare February 3, 2022 10:09

shaavan force-pushed the 220112-chrono branch from cad0e0a to 88c5b10 Compare February 3, 2022 11:38

shaavan force-pushed the 220112-chrono branch from 88c5b10 to f7a19ef Compare February 4, 2022 10:10

hebasto merged commit 5c6b3d5 into bitcoin-core:master Feb 4, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 6, 2022

Merge bitcoin-core/gui#524: Replace int with std::chrono in for the t…

6152180

shaavan deleted the 220112-chrono branch February 9, 2022 10:37

bitcoin-core locked and limited conversation to collaborators Feb 9, 2023

Comments

Conversation

shaavan commented Jan 12, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shaavan commented Jan 13, 2022

Uh oh!

DrahtBot commented Jan 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shaavan commented Jan 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shaavan commented Jan 18, 2022

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

RandyMcMillan commented Jan 18, 2022

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Jan 20, 2022

Choose a reason for hiding this comment

Uh oh!

RandyMcMillan Jan 20, 2022

Choose a reason for hiding this comment

Uh oh!

RandyMcMillan Jan 20, 2022

Choose a reason for hiding this comment

Uh oh!

shaavan Jan 21, 2022

Choose a reason for hiding this comment

Uh oh!

RandyMcMillan Jan 28, 2022

Choose a reason for hiding this comment

Note

Uh oh!

shaavan commented Jan 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shaavan commented Jan 27, 2022

Uh oh!

RandyMcMillan commented Jan 28, 2022

Note

Uh oh!

shaavan commented Jan 31, 2022

Uh oh!

RandyMcMillan commented Feb 1, 2022

Uh oh!

shaavan commented Feb 1, 2022

DrahtBot commented Jan 13, 2022 •

edited

Loading

jonatack left a comment •

edited

Loading

shaavan commented Jan 16, 2022 •

edited

Loading

hebasto left a comment •

edited

Loading

shaavan commented Jan 21, 2022 •

edited

Loading

shaavan commented Feb 3, 2022 •

edited

Loading