Skip to content

Add an option to hide or show Cache and Buffers in Graph mode#1540

Merged
fasterit merged 4 commits intohtop-dev:mainfrom
batk0:main
Jan 8, 2025
Merged

Add an option to hide or show Cache and Buffers in Graph mode#1540
fasterit merged 4 commits intohtop-dev:mainfrom
batk0:main

Conversation

@batk0
Copy link
Copy Markdown
Contributor

@batk0 batk0 commented Sep 25, 2024

This PR covers issue #407.

[x]    Show cached memory in graph

Added this option to "Display Options". It's enabled by default to be compatible with current behavior. Disabling it will remove Cache and Buffers memory from Memory Graph and Mem&Swp Graph.

@Explorer09
Copy link
Copy Markdown
Contributor

I expect this PR be rebased on / merged with #928. Otherwise I fear this would conflict with the multi-color graph feature I already proposed in #714.

@batk0
Copy link
Copy Markdown
Contributor Author

batk0 commented Sep 25, 2024

I expect this PR be rebased on / merged with #928. Otherwise I fear this would conflict with the multi-color graph feature I already proposed in #714.

No problem. I can rebase as soon as #928 is merged. Otherwise I don't know what to do.

@BenBE
Copy link
Copy Markdown
Member

BenBE commented Sep 26, 2024

The actual change looks good (should be one commit though, not two), but I'm not very eager about the feature overall.

@fasterit @natoscott What do you think?

@fasterit
Copy link
Copy Markdown
Member

I think for consistency reasons this switch should also affect the bar mode.
I'm not against it, it's small and fits a use case of seeing how much memory is used and not-easily evictable.
/DLange

Copy link
Copy Markdown
Member

@fasterit fasterit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said on a comment this should also affect bar mode.

The cheap "NaN" stunt causes an assert failure:

stderr output >>>>>>>>>>
htop: Meter.c:409: Meter_humanUnit: Assertion `value >= 0.0' failed.
<<<<<<<<<< stderr output <<<<<<<<<<

The code also needs to save the setting to htoprc and print it in the crash handler (the Settings handling is incomplete).

@natoscott
Copy link
Copy Markdown
Member

@fasterit @natoscott What do you think?
I'm not against it, it's small and fits a use case [...]

I don't love it, as it adds yet another configurable option many people wont need and the Options menu is already cluttered. OTOH I don't see any better solution, so I wont object to it being added.

@batk0 batk0 requested a review from fasterit January 8, 2025 05:42
@Explorer09
Copy link
Copy Markdown
Contributor

@fasterit Merged too fast. I think some of the commits should have been squashed before merging. Otherwise we would have small fixup commits forever in the history of main. (It's not my call, but I just suggest to be careful next time.)

@BenBE
Copy link
Copy Markdown
Member

BenBE commented Jan 8, 2025

I cleaned up the history (squashing the merge + amended commit). AFAICS only one fork (that I follow/fetch) pulled that commit by the time I force-pushed the updated tip.

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.

5 participants