Skip to content

Mirrored Input/Output Graph Mode #1701

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

BenBE
Copy link
Member

@BenBE BenBE commented May 15, 2025

This extends the existing graph mode to allow for vertically mirrors graphs representing input and output rates.

image

This basically implements the idea from #1390 based on the #1387 that enables only specific meters to implement these additional modes.

@Explorer09 This is what I meant with code re-use … Most of the added code is from additional constants, with most of the existing code re-used with only minor changes.

NB: There's some known bugs as I needed to slightly change the memory layout for the Meter->values data storage for the DiskIO/NetworkIO meters, which causes issues in bar meter mode.

@BenBE BenBE added this to the 3.5.0 milestone May 15, 2025
@BenBE BenBE added needs-discussion 🤔 Changes need to be discussed and require consent new feature Completely new feature labels May 15, 2025
@BenBE BenBE linked an issue May 15, 2025 that may be closed by this pull request
@Explorer09
Copy link
Contributor

@Explorer09 This is what I meant with code re-use … Most of the added code is from additional constants, with most of the existing code re-used with only minor changes.

I'm not a fan of this way of changing code. Expect for the addition of a new meter mode, I have different opinions on how the code can be done.

Firstly:

There's some known bugs as I needed to slightly change the memory layout for the Meter->values data storage for the DiskIO/NetworkIO meters, which causes issues in bar meter mode.

We're now displaying two different kinds of data for the DiskIO meter. I suggest splitting the meter into two. The meter named DiskIO keeps the current behavior of displaying the utilization percentage, and the other, which I would name DiskIOSpeed for now, shows the read and write speeds. This way we don't have to use different modes to display different kinds of data.

For NetworkIO meter, there's no need to split.

Second. I don't like the GraphMeterMode_drawGraph the way you implemented it.

https://github.com/htop-dev/htop/pull/1701/files#diff-9ea32cfb9d3bca3e912da3efac9082542fb87f3689561174d2e18bf33a07dae3R353-R358

A few issues: (a) it works with GRAPH_HEIGHT that is even only (my #1415 code did address the case where the height is odd); (b) the new channel argument for the function is a control coupling that I wish to avoid.

My vision was the GraphMeterMode_draw function be used for both meter modes (the original Graph and the new mode that you called IO), and within the draw function the meter's mode property is read, which determines how the dots should be drawn. As I have written #714, I considered that the new feature code might be more maintainable if I base #1415 on #714. And that was how it went.

And there are two more minor things:

@BenBE
Copy link
Member Author

BenBE commented May 16, 2025

No objection on cherry-picking c7c0cda … That one can go right in AFAICS. Though I had to do some minor adjustments.

@BenBE BenBE force-pushed the in-out-graph branch 2 times, most recently from d7120dc to e01e11e Compare May 16, 2025 07:13
@BenBE
Copy link
Member Author

BenBE commented May 16, 2025

Regarding the name, I'm not particular focused on the name and looking at what these kinds of graphs are commonly referred to Bipolar sounds like a viable option/alternative to me.

Regarding the approach:
My approach focuses on re-using as much of the existing code as possible, adding small parameters for where different behaviour is required. My approach also avoids quite a large chunk of "derive some UTF-8 sequence" code you use for determining the character to place on the graph.

Splitting the DiskIO meter into one for utilization and another for bandwidth might be an option; but as you see in my PR: That's just a matter of updating these meters to either support this graph style or not.

The major issue I really see with #714 and thus also #1415 is the very huge upfront code change that introduces quite complex code. OTOH this PR comes with very little up-front changes and even the changes it makes are easy to understand. The time it took me to write this PR is roughly the same it took me to go through #714's dot placing function; and while I'm kinda confident with the code in this PR I won't really say the same for #714.

@Explorer09
Copy link
Contributor

@BenBE Did you consider incorporating the "dynamic scaling" part of #714? I think that part would be needed for your changes here. Specifically these:

596fa20 Introduce isPercentChart member in MeterClass
0995f49 Update "total" value for non-percent bar meters
e237eed Graph meter dynamic scaling and percent graph drawing
f7fee7f Adjust LoadAverageMeter "total" assignment
42c111e TasksMeter: remove code for auto-updating "total"
7ae110b NetworkIOMeter: remove code for auto-updating "total"

I'm not sure if I need to file a separate PR for that.

The reason is something to do with the "total" value: The DiskIO and NetworkIO meters you are working on do not have a maximum of the read and write speeds. With the above patches, you can have "dynamic total" values that work consistently across different kinds of meters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-discussion 🤔 Changes need to be discussed and require consent new feature Completely new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Graph meter style with negative values
2 participants