Enhanced Traffic Graph Widget with Multi-timeframe Support and Data Persistence #866
Conversation
rebroad
force-pushed
the
trafficgraphwidget-rebased
branch
from
530adae to
31b1ffc
Compare
rebroad
changed the title
luke-jr
left a comment
luke-jr
left a comment
There was a problem hiding this comment.
Merging it all together likes this makes review very annoying. It was better as separate PRs.
src/qt/trafficgraphwidget.cpp
Outdated
| // Restore the saved total bytes counts to the node if they exist | ||
| if (m_total_bytes_recv > 0 || m_total_bytes_sent > 0) { | ||
| model->node().setTotalBytesRecv(m_total_bytes_recv); | ||
| model->node().setTotalBytesSent(m_total_bytes_sent); | ||
| } |
There was a problem hiding this comment.
The GUI should not be changing low-level internals.
If for some reason the clientModel is changed, its data should be taken as authoritative, even if it contradicts the history so far (if that's an issue, wipe the history).
Alternatively, maybe the history should be stored with the node, and loaded into the GUI (or RPC, in another hypothetical PR).
There was a problem hiding this comment.
The idea is that the data is not persistent - but yes, I see your point - it should probably not be something that only happens when the GUI is running, and should perhaps also happen in bitcoind also.
There was a problem hiding this comment.
The GUI should not be changing low-level internals.
If for some reason the clientModel is changed, its data should be taken as authoritative, even if it contradicts the history so far (if that's an issue, wipe the history).
Alternatively, maybe the history should be stored with the node, and loaded into the GUI (or RPC, in another hypothetical PR).
@luke-jr I've removed the setTotalBytes functions as I agree with your reasoning. The GUI keeps a local track of the totals from the last session and simply adds them to the totals provided by the node now. I did think about storing the data on the node, but I am not sure it makes much sense given the data is currently stored on the GUI and so it makes more sense to keep the totals local with the traffic history for now. Perhaps someone thinks it's worth storing this on the node in the future, but it doesn't seem like an urgent need for now.
| </widget> | ||
| </item> | ||
| <item> | ||
| <widget class="QPushButton" name="btnClearTrafficGraph"> |
There was a problem hiding this comment.
If we're deleting this, maybe there should be a way for the user to insert a reference line?
For now, I'd move removal of anything to a separate PR.
There was a problem hiding this comment.
The user can effectively remove the data by deleting the .dat file - is there any basis for being able to do this while the client is running?
| // based timer interface | ||
| m_node.rpcSetTimerInterfaceIfUnset(rpcTimerInterface); | ||
|
|
||
| setTrafficGraphRange(INITIAL_TRAFFIC_GRAPH_MINS); |
There was a problem hiding this comment.
Why are we losing this default constant?
There was a problem hiding this comment.
because it isn't needed - on startup it "defaults" to the first non-full range
I vaguely remember people complaining that I was raising several PRs that could be combined when I first raised the PRs years ago.... I guess one cannot please everyone all of the time. @luke-jr As a middle-ground, I can move the distinct functional changes into different commits. |
rebroad
force-pushed
the
trafficgraphwidget-rebased
branch
from
31b1ffc to
ab7bbd0
Compare
laanwj
changed the title
rebroad
force-pushed
the
trafficgraphwidget-rebased
branch
6 times, most recently
from
280b15c to
de2f01a
Compare
|
I've removed the code that changed the totals on the node - the GUI just keeps track now without writing values to the node. Also, added some checks for corrupt save files so that it doesn't load bad data. |
rebroad
force-pushed
the
trafficgraphwidget-rebased
branch
3 times, most recently
from
0982a88 to
47732ed
Compare
|
I realise this PR is starting to look a bit messy with all the updates. I also realise I ought to separate various functionality changes into separate commits. Is it better to raise a new PR once this is done, force push to this one? |
rebroad
force-pushed
the
trafficgraphwidget-rebased
branch
6 times, most recently
from
4d14caf to
54d90c9
Compare
rebroad
force-pushed
the
trafficgraphwidget-rebased
branch
from
54d90c9 to
28ac517
Compare
|
Have just added some more GUI improvements - the text on the graph now has a black outline around it making it easier to read (and is printed AFTER the graph rather than before) .Also, the grey line at the bottom of the graph is now printed after the graph also, causing the graph not to spill onto that line. Also, the bright green and red lines no longer surround the sides of the graph but only draw an outline along actual graph values. This makes it easier to distinguish about the graph going off the edge of the display areas, vs a sudden drop in traffic (which previously looked the same). The now graph also fills the area fully to the edges at the side now (which it did not before due to rounding issues). It would be useful to know whether I should split all these changes into individual pull requests, or individual commits, or better all together like this. |
rebroad
force-pushed
the
trafficgraphwidget-rebased
branch
from
28ac517 to
c0b9aff
Compare
rebroad
force-pushed
the
trafficgraphwidget-rebased
branch
10 times, most recently
from
b1aa3b8 to
a896748
Compare
Add a new GUI utility function to format bytes per second values with appropriate units (B/s, kB/s, MB/s, GB/s) and precision, ensuring consistent and readable display of network traffic rates in the traffic graph widget.
Add a new time formatting function that matches the existing date and datetime ISO8601 formatting functions, providing consistent time display for the traffic graph widget tooltips and other UI elements that need time-only display.
rebroad
force-pushed
the
trafficgraphwidget-rebased
branch
from
a896748 to
15fd680
Compare
…persistence
This commit significantly improves the network traffic graph widget with:
1. Multiple timeframe support - View traffic data across different time
periods (5 minutes to 28 days) using an enhanced slider interface
2. Traffic data persistence - Save and restore traffic information
between sessions, preserving historical traffic patterns
3. Interactive visualization features:
- Logarithmic scale toggle (mouse click) for better visualization of
varying traffic volumes
- Interactive tooltips showing detailed traffic information at specific points
- Yellow highlight indicators for selected data points
4. Smooth transitions between different time ranges with animated scaling
These improvements allow users to better monitor and analyze network
traffic patterns over time, which is especially useful for debugging
connectivity issues or understanding network behavior under different
conditions.
The implementation includes proper thread-safety considerations and
handles edge cases like time jumps or missing data appropriately.
rebroad
force-pushed
the
trafficgraphwidget-rebased
branch
from
15fd680 to
bc13d01
Compare
|
concept ACK |
5 participants
This PR improves the network traffic graph widget in the Debug window to provide:
Motivation
The existing network traffic graph has limited utility with its fixed time range and lack of historical data preservation. This enhancement allows users to:
These improvements are valuable for:
Implementation
The implementation preserves all existing functionality while adding new features:
Supporting changes:
Testing
Tested on Linux with various network conditions. The new functionality can be exercised by:
Documentation
The changes are mostly self-documenting through the UI and are constrained to the Qt interface without affecting core functionality.
Compatibility
This PR maintains compatibility with existing functionality. The data persistence file uses proper serialization versioning to allow for future format changes if needed.