Skip to content
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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

acxz
Copy link

@acxz acxz commented Apr 11, 2022

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:
cpu_unsort

With cpu_sort: true:
cpu_sort

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 with cpu_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:

  • Windows
  • macOS
  • Linux

Checklist

If relevant, ensure the following have been met:

  • Areas your change affects have been linted using rustfmt (cargo fmt)
  • The change has been tested and doesn't appear to cause any unintended breakage
  • Documentation has been added/updated if needed (README.md, help menu, etc.)
  • The pull request passes the provided CI pipeline
  • There are no merge conflicts

@ClementTsang ClementTsang self-requested a review April 11, 2022 05:55
@codecov-commenter
Copy link

codecov-commenter commented Apr 11, 2022

Codecov Report

Merging #703 (dc6d508) into master (6fa04de) will decrease coverage by 0.04%.
The diff coverage is 4.16%.

@@            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     
Impacted Files Coverage Δ
src/app.rs 0.00% <0.00%> (ø)
src/app/data_farmer.rs 0.00% <0.00%> (ø)
src/bin/main.rs 0.00% <0.00%> (ø)
src/canvas/dialogs/dd_dialog.rs 0.00% <0.00%> (ø)
src/canvas/widgets/battery_display.rs 0.00% <0.00%> (ø)
src/canvas/widgets/cpu_basic.rs 0.00% <0.00%> (ø)
src/canvas/widgets/cpu_graph.rs 0.00% <0.00%> (ø)
src/canvas/widgets/disk_table.rs 0.00% <0.00%> (ø)
src/canvas/widgets/mem_graph.rs 0.00% <0.00%> (ø)
src/canvas/widgets/network_graph.rs 0.00% <0.00%> (ø)
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 194a769...dc6d508. Read the comment docs.

@ClementTsang
Copy link
Owner

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.

@ClementTsang
Copy link
Owner

ClementTsang commented Apr 12, 2022

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 - cpu_sort, at least personally, made me think at first that it would just sort the CPU core entries by current usage, not just the values. As in, if CPU0 was at 10% while CPU10 was at 50%, the entire CPU10 entry, along with its usage, would be put above CPU0 *. So I'm worried the option name might be misleading. Again, I don't really have any actual suggestions at the moment, unfortunately.

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.

@acxz
Copy link
Author

acxz commented Apr 12, 2022

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)

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 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 Rank instead of just CPU and RankX instead of CPUX?

would also consider renaming the option as well - cpu_sort

maybe sort_cpu_hist(or usage) is a better flag here if we want sort_cpu to be for the current utilization value.

As in, if CPU0 was at 10% while CPU10 was at 50%, the entire CPU10 entry, along with its usage, would be put above CPU0 *.

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 sort_cpu flag while we finalize a name and better UI/UX for this feature.

As for the code/logic itself, it looks fine to me right now, so good work there.

Thanks!

Edit: I have renamed the option to sort_cpu_hist and changed the legend entries to use Rank instead of CPU. Let me know if you have a better suggestion.

@acxz acxz mentioned this pull request Apr 23, 2022
8 tasks
@acxz acxz changed the title add option to sort cpus by values add option to sort cpu history Apr 23, 2022
@magnetophon
Copy link

I came here to file an issue about this and saw there's already a PR.
I'd love to see this merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to show sorted CPU values
4 participants