clean up some drawing code, consolidate time graph components #710
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ClementTsang
force-pushed
the
consolidate_component_drawing
branch
3 times, most recently
from
73a4f04
to
63096aa
Compare
Codecov Report
@@ Coverage Diff @@
## master #710 +/- ##
==========================================
+ Coverage 17.14% 20.85% +3.71%
==========================================
Files 52 53 +1
Lines 13215 13630 +415
==========================================
+ Hits 2266 2843 +577
+ Misses 10949 10787 -162
Continue to review full report at Codecov.
|
ClementTsang
force-pushed
the
consolidate_component_drawing
branch
2 times, most recently
from
a8a32da
to
db89cad
Compare
ClementTsang
force-pushed
the
consolidate_component_drawing
branch
from
db89cad
to
34d2a16
Compare
When I was newer to Rust, I got the weird impression that you couldn't add functionality to a struct outside of the defining file without using a trait. That's obviously not true, so it's high time I got rid of it and just made it part of the impl of the class itself, rather than declaring a trait and then exporting/importing it.
This code was never used and might as well be removed for clarity's sake.
ClementTsang
force-pushed
the
consolidate_component_drawing
branch
4 times, most recently
from
9850810
to
f72fd9d
Compare
This consolidates all the time graph drawing to one main location, as well as some small improvements. This is helpful in that I don't have to reimplement the same thing across three locations if I have to make one change that in theory should affect them all. In particular, the CPU graph, memory graph, and network graph are all now using the same, generic implementation for drawing, which we call (for now) a component. Note this only affects drawing - it accepts some parameters affecting style and labels, as well as data points, and draw similarly to how it used to before. Widget-specific actions, or things affecting widget state, should all be handled by the widget-specific code instead. For example, our current implementation of x-axis autohide is still controlled by the widget, not the component, even if some of the code is shared. Components are, again, only responsible for drawing (at least for now). For that matter, the graph component does not have mutable access to any form of state outside of tui-rs' `Frame`. Note this *might* change in the future, where we might give the component state. Note that while functionally, the graph behaviour for now is basically the same, a few changes were made internally other than the move to components. The big change is that rather than using tui-rs' `Chart` for the underlying drawing, we now use a tweaked custom `TimeChart` tui-rs widget, which also handles all interpolation steps and some extra customization. Personally, I don't like having to deviate from the library's implementation, but this gives us more flexibility and allows greater control. For example, this allows me to move away from the old hacks required to do interpolation (where I had to mutate the existing list to avoid having to reallocate an extra vector just to insert one extra interpolated point). I can also finally allow customizable legends (which will be added in the future).
ClementTsang
force-pushed
the
consolidate_component_drawing
branch
from
f72fd9d
to
2401e58
Compare
15 tasks
ClementTsang
added a commit
that referenced
this pull request
May 17, 2022
This serves as somewhat of an intermediary refactor to unify some scrollable table code - in particular, in regards to drawing. This is almost a parallel refactor as #710, which did something similar for time graphs. However, this one has a bit more work in regards to the concepts of component state, in particular, for width calculation caching and scroll position management.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
A description of the change and what it does. If relevant (such as any change that modifies the UI), please provide screenshots of the change:
Cleans up some drawing code and unifies all time graph drawing.
Issue
If applicable, what issue does this address?
Closes: #
Testing
If relevant, please state how this was tested. All changes must be tested to work:
Please also indicate which platforms were tested. All platforms directly affected by the change must be tested:
Checklist
If relevant, ensure the following have been met:
cargo fmt)README.md, help menu, etc.)