-
Notifications
You must be signed in to change notification settings - Fork 252
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
add option to sort cpu history #703
base: main
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #703 +/- ##
=========================================
- Coverage 7.84% 7.80% -0.05%
=========================================
Files 53 53
Lines 8269 8316 +47
=========================================
Hits 649 649
- Misses 7620 7667 +47
Continue to review full report at Codecov.
|
I'll try to take a look at this in a bit. Don't worry about the failed codecov check, it's fine for now. |
Took a bit of a look and tried it on my machine. One thing I'm a bit concerned about is that it might be a bit confusing that, say, "CPU0" now means something entirely different. With the sort off, it means the 1st core/thread. But with it on, now it just means "the highest usage". This is kinda misleading to me. It might be a bit tricky since my UI code right now is admittedly pretty awful (hopefully I will have time to finally work on it after I graduate soon), but it might be better to rename the entries and/or column if this option is enabled to be better indicative that the first entry represents the highest usages over all threads, second entry represents second-highest usage over all threads, etc. As for what to call this column, I don't know off the top of my head. I would also consider renaming the option as well - As for the code/logic itself, it looks fine to me right now, so good work there. I feel like the main things is just hammering out any UI/UX details. If I have any ideas, I'll put them below (maybe in a week or so after I finish some IRL stuff), but feel free to leave any thoughts. * This is behaviour I might plan on adding in the future, for what its worth. |
Thanks for the review @ClementTsang. I agree with you on your points and had the same concerns/doubts while implementing this feature (i.e. how to best present it)
I like the renaming idea, but again not sure what to rename it yet. If we can change the legend header than it can give us a better UI/UX option. maybe something like
maybe
I actually already implemented this first (as a stepping stone for this feature), and you can see it in the commit history: 228292d Note: while the legend is fine, this makes the graphs nigh impossible to interpret as entire lines switch colors constantly at every timestep. if you want I can separate that commit out and submit it as a PR enabled with the
Thanks! Edit: I have renamed the option to |
I came here to file an issue about this and saw there's already a PR. |
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:
This patchset adds the option to sort cpus by value.
With
cpu_sort: false
:With
cpu_sort: true
:Issue
If applicable, what issue does this address?
Closes: #702
Testing
If relevant, please state how this was tested. All changes must be tested to work:
Tested with
cpu_sort: false
to retain default behavior, tested withcpu_sort: true
to show ordered CPU values in cpu_legend and ordered/non-intersecting lines in cpu_graph.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.)