Per-meter display types

[BenBE]

This PR restricts the list of available Meter display options to the ones that are sensible in the context of each meter (AKA "supported").

Given that some meters mostly represent textual information anyway (Hostname, System Information) or have monotonically increasing values anyway (Clock, Date, DateTime) there's not much sense, providing graphs for these.

Another application for this PR is regarding #714 to provide a multi-graph option for Network and Disk I/O. This would allow to show a special graph that displays a second data series downwards instead of stacked on top (@eworm-de @Explorer09 : That's what I'd prefer for Disk IO and Network IO meters at least).

[Explorer09]

1. @BenBE , I don't know what your idea is regarding to showing "a special graph that displays a second data series downwards instead of stacked on top".
2. Note that my graph meter algorithm in (Dynamic scaling & Graph meter coloring (new) #714) is intended to work as a generic "stacked" graph, without tuning to any specific meter. As long as the data make sense when presented as a stacked bar, it can be rendered as a graph (with little visual errors).
3. For Clock, Date and DateTime meters, I agree that bar and graph may be removed from them. I would also add Uptime meter to the list.
4. While it is a good idea to remove support of modes (i.e. styles) of certain meters, I don't like the way it is implemented in this PR. My comments are these:
   (a) It doesn't make sense to separate the support bits of bar mode and graph mode. The reason is what I mentioned above: My graph algorithm is intended to be generic.
   (b) To disable bar and graph mode rendering, my preferred way is to check MeterClass.maxItems == 0. That way no additional flag is needed.
   (c) Disabling LED meter mode may be achieved with a boolean flag.
   (d) I don't see your PR addresses anything about CUSTOM_METERMODE.

[eworm-de]

I think the idea for "a special graph that displays a second data series downwards instead of stacked on top" is something like this:

(Just a mock from Gimp, no code exists.)

[BenBE]

1. @BenBE , I don't know what your idea is regarding to showing "a special graph that displays a second data series downwards instead of stacked on top".

Foo:⠀⠀⠀⠀⠀⡀⡄⠀⠀⣠⣧
    ⣀⣀⣀⣀⣄⣷⣷⣶⣾⣿⣿
    ⠉⠉⠋⠉⠉⠋⠏⠛⠛⠟⣿
    ⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠹

Available characters (Unicode):

⠀⢀⡀⣀⢠⡄⣠⣄⣤⢰⡆⣰⣆⣴⣦⣶⢸⡇⣸⣇⣼⣧⣾⣷⣿
⠀⠈⠁⠉⠘⠃⠙⠋⠛⠸⠇⠹⠏⠻⠟⠿⢸⡇⢹⡏⢻⡟⢿⡿⣿

Kinda inspired by graphs like this:

2. Note that my graph meter algorithm in (Dynamic scaling & Graph meter coloring (new) #714) is intended to work as a generic "stacked" graph, without tuning to any specific meter. As long as the data make sense when presented as a stacked bar, it can be rendered as a graph (with little visual errors).

I'm aware of that. Just noting that for some information (like Disk IO and Network Bandwidth) a simple in/out display (cf. first item in this list) makes more sense.

It's not as if you can't have both #714 style graphs and simple in/out graphs for meters where they make sense. What this PR mostly does is to allow meters to disable display formats that just don'e make sense (like graph display on the SysArch meter).

3. For Clock, Date and DateTime meters, I agree that bar and graph may be removed from them. I would also add Uptime meter to the list.

ACK, missed that one in my listing above … In the PR it's already included AFAICS.

4. While it is a good idea to remove support of modes (i.e. styles) of certain meters, I don't like the way it is implemented in this PR. My comments are these:
   (a) It doesn't make sense to separate the support bits of bar mode and graph mode. The reason is what I mentioned above: My graph algorithm is intended to be generic.

As I'd suggest to keep the old style, the one implemented in #714 simple would become a new bit (METERMODE). Either in the default set (cf. #define in Meter.h) or for each meter where it's appropriate, you'd just add that bit in the support mask. The current implementation mostly masks bits for things where the default set is too broad, thus adding bits in the default support set would automatically add your meter for all meters that already support the graph style anyway.

(b) To disable bar and graph mode rendering, my preferred way is to check MeterClass.maxItems == 0. That way no additional flag is needed.

This makes for very interlinked code when additional constraints emerge. This also doesn't cover the clock/date meters as they contain the time of day as one item value.

(c) Disabling LED meter mode may be achieved with a boolean flag.

Whether LED mode makes sense or not is mostly dependent on the displayed text. For Clock/Date/Uptime meters it's sensible, while for the SysArch meter it's not. Having this handled in a "support flag set" has the big advantage that you can simply handle switching modes in one place. Having additional flags would require MeterPanel.c to special-case each additional such constraint.

(d) I don't see your PR addresses anything about CUSTOM_METERMODE.

CUSTOM_METERMODE is defined as 0 per Meter.h. If modeIndex < 1 you return from Meter_setMode early. No change in my code in that case. Or am I missing something here?

[BenBE]

I think the idea for "a special graph that displays a second data series downwards instead of stacked on top" is something like this:

(Just a mock from Gimp, no code exists.)

Looks as envisioned*. Mind to build a PR from this?

*except for the "pixel" gaps …

[Explorer09]

Looks as envisioned*. Mind to build a PR from this?

*except for the "pixel" gaps …

The "pixel gaps" were from the bug I left in #714 .
While I have the idea for your proposal now (which I would call "two quadrant" display of the graph), I still have some concerns about it before I can try implementing it. The code in #714 can be extended to cover this display mode with little difficulty. The question would be how useful this display mode could be for all meters in general.

1. I personally don't like implementing a display mode of the graph that is meter specific, because that means additional code for the graph drawing algorithm with little benefit.
2. The "two quadrant" display of the graph doesn't align well with the current bar display of the meter (which still shows stacked bars despite the feature).
3. I need to set some constraints for this new mode before it turns into a scope creep or an algorithm I couldn't maintain further:
4. Scale constraint. The display scale would have to be the same for the upper half (quadrant) and lower half (quadrant) of the graph.
5. Number of items constraint. Only meters with one or two items (MeterClass.maxItems < 2) may be displayed in this mode. There's can't be items stacked on either side (upper or lower) as the "stacked" color data structure in Dynamic scaling & Graph meter coloring (new) #714 would be incompatible when item stacking happens on both sides.
6. The "time" axis, i.e. the zero value line, might have to stay in the middle of the graph. It would be unable to move up or down.

[BenBE]

Looks as envisioned*. Mind to build a PR from this?
*except for the "pixel" gaps …

The "pixel gaps" were from the bug I left in #714 .

I guessed so. Just remarked on it for completeness.

While I have the idea for your proposal now (which I would call "two quadrant" display of the graph), I still have some concerns about it before I can try implementing it. The code in #714 can be extended to cover this display mode with little difficulty. The question would be how useful this display mode could be for all meters in general.

I can see this meter mode for at least Disk I/O, Network I/O; possibly other meters where the notion of two opposite directions of flow/values makes sense. But TBH, even if this mode was only used for like two metrics, this could already be quite useful.

1. I personally don't like implementing a display mode of the graph that is meter specific, because that means additional code for the graph drawing algorithm with little benefit.

No need: Just grab the first two values and display them. Nothing you couldn't reuse with the "stacked" mode, where instead of only the first two you'd just use all of them.

2. The "two quadrant" display of the graph doesn't align well with the current bar display of the meter (which still shows stacked bars despite the feature).

Shouldn't be too hard to figure something for that case.

3. I need to set some constraints for this new mode before it turns into a scope creep or an algorithm I couldn't maintain further:
4. Scale constraint. The display scale would have to be the same for the upper half (quadrant) and lower half (quadrant) of the graph.

ACK. Would be confusing otherwise. Could even be log10 per (Braille) pixel, with the "zero line" always drawn. Gives 7 orders of magnitude for display and with 10K being the baseline (unless there's need to scale further) could display a full-duplex 1GbE link.

5. Number of items constraint. Only meters with one or two items (MeterClass.maxItems < 2) may be displayed in this mode. There's can't be items stacked on either side (upper or lower) as the "stacked" color data structure in Dynamic scaling & Graph meter coloring (new) #714 would be incompatible when item stacking happens on both sides.

I'd even argue only meters with two items should use this meter.

6. The "time" axis, i.e. the zero value line, might have to stay in the middle of the graph. It would be unable to move up or down.

ACK. NP. That's how I'd've gone about it too.

NB: Maybe split further discussion into it's own issue/PR, so we can stick to the meter mode rework in this PR.

[Explorer09]

NB: Maybe split further discussion into it's own issue/PR, so we can stick to the meter mode rework in this PR.

Sure.

(b) To disable bar and graph mode rendering, my preferred way is to check MeterClass.maxItems == 0. That way no additional flag is needed.
This makes for very interlinked code when additional constraints emerge. This also doesn't cover the clock/date meters as they contain the time of day as one item value.

My idea was to remove the item values totally, so that Meter.values would become null pointer for clock/date/uptime meters, and all other meters that do not need the bar display mode.

Whether LED mode makes sense or not is mostly dependent on the displayed text. For Clock/Date/Uptime meters it's sensible, while for the SysArch meter it's not. Having this handled in a "support flag set" has the big advantage that you can simply handle switching modes in one place. Having additional flags would require MeterPanel.c to special-case each additional such constraint.

CUSTOM_METERMODE is defined as 0 per Meter.h. If modeIndex < 1 you return from Meter_setMode early. No change in my code in that case. Or am I missing something here?

My point is you already special-cased CUSTOM_METERMODE. If we want to handle "supported meter modes" in one central bit mask, then meters with CUSTOM_METERMODE only should apply too. Otherwise it becomes two pieces of code for handling the same thing, and the bit mask won't be worth it.

[BenBE]

(b) To disable bar and graph mode rendering, my preferred way is to check MeterClass.maxItems == 0. That way no additional flag is needed.
This makes for very interlinked code when additional constraints emerge. This also doesn't cover the clock/date meters as they contain the time of day as one item value.

My idea was to remove the item values totally, so that Meter.values would become null pointer for clock/date/uptime meters, and all other meters that do not need the bar display mode.

ACK.

CUSTOM_METERMODE is defined as 0 per Meter.h. If modeIndex < 1 you return from Meter_setMode early. No change in my code in that case. Or am I missing something here?

My point is you already special-cased CUSTOM_METERMODE. If we want to handle "supported meter modes" in one central bit mask, then meters with CUSTOM_METERMODE only should apply too. Otherwise it becomes two pieces of code for handling the same thing, and the bit mask won't be worth it.

Can you elaborate? I didn't touch anything related to the CUSTOM_METERMODE that has already been there before.

[Explorer09]

Allow me to restate my concerns about this PR?

Generally I don't like the idea of having a per-meter supportedMode flags.

1. There are only 5 meter modes for htop now (including CUSTOM_METERMODE), with the feature discussion in Graph meter style with negative values #1390 being the potential 6th mode. And I think for every mode, whether it is supported by the meter (class) can be determined in an automated manner.

• CUSTOM_METERMODE: With MeterClass.defaultMode being CUSTOM_METERMODE, it is unlikely that the meter can support any other mode.
• BAR_METERMODE: Can determine its support by MeterClass.maxItems > 0
• TEXT_METERMODE: Always supported except for CUSTOM_METERMODE meters.
• GRAPH_METERMODE: Can determine its support by MeterClass.maxItems > 0 (basically whatever can be drawn as a bar can also be drawn as a graph)
• Second graph mode in Graph meter style with negative values #1390 can be determined by (MeterClass.maxItems > 0 && MeterClass.maxItems <= 2)
• Perhaps LED_METERMODE is the only mode where it's useful to control its support per meter. But it wouldn't hurt if we leave it alone -- as LED meter mode is just an alternative to Text mode display (with digits shown as big characters).

2. Meters with CUSTOM_METERMODE (which are essentially "containers" of meters) would need special handling anyway in Meter_setMode() function even if we have supportedMode flags implemented. (This limits the benefit of supportedMode as a general bit mask.)

And finally I don't think there would be a lot of new meter modes in htop. (Maybe "micro-text" mode and "Graph2" mode, but probably not more.)

[BenBE]

I think, even for graph mode 1a/1b vs. 2 it's not that clear cut even though the gm2 is mostly a special case. Not all data you can stack in modes 1a/1b makes sense as an in/out style graph in mode 2.

Re 1a/1b/2: Mode 1a is basically the current implementation with the graph just showing the total value over time and 1b being the new implementation in #714. Mode 2 being #1390. They are distinct and so is their set of use cases.

I'll have to take another look at the ustom meter modes, but AFAIR the only use I currently remember is with the CPU meters; and those allow for text. Will have to take another look there, but I doubt that's something that fully breaks this usage.

[Explorer09]

@BenBE When I wrote #714 I intended the mode to replace the current graph mode implementation, not as an alternative "1a/1b" as you say.

I think you are referring to #928 instead. When a meter supports a single "summary" value, we can plot that one instead of the stacked colors.

With #928 in mind, there would be three graph display modes:

• "Graph-Summary" mode: Introduce "single value" model for monochrome graph #928. Plot Meter.summaryValue only, or if (MeterClass.maxItems == 1), plot the only value data of the meter.
• "Graph-Stacked" mode: Basically what Dynamic scaling & Graph meter coloring (new) #714 does.
• "Graph-Negative" mode: (Or "Graph-2-Quadrant" mode) Graph meter style with negative values #1390. Only supported if (MeterClass.maxItems <= 2). There is potential for plotting a negative meter value with this mode, but for plotting things like temperatures (°C), the visual would not look ideal.

[BenBE]

Another aspect for this PR is that "per mode checks" are difficult to iterate through automatically. Also, basically we are already using an implicit bitmap of "all supported" right now.

[Explorer09]

I'm curious about one thing. Since this patch would disable certain modes for certain meters, what would happen if the user has an htop configuration that sets a meter to a mode that is no longer "supported" in the newer version of htop?

[BenBE]

I'm curious about one thing. Since this patch would disable certain modes for certain meters, what would happen if the user has an htop configuration that sets a meter to a mode that is no longer "supported" in the newer version of htop?

With the new patch htop would silently select the next supported mode in order.

[Explorer09]

I have some experimental changes that you could consider merging with this feature. See this branch:
https://github.com/Explorer09/htop-1/tree/meter-next-mode

In particular, these two commits:

• Remove CUSTOM_METERMODE special handling
• Meter_nextSupportedMode() function

[BenBE]

Those patches overall LGTM. Some changes:

1. Squash 50c9058 into d70b8cc.
2. In d70b8cc split the change in Meter.h (and related changes).
3. Commit d70b8cc should be split into the changes related to defaultMode and those of the CPUMeter_init refactor

[BenBE]

@Explorer09 Please check this updated patchset. I think there broke something on rebase.

[BenBE]

@Explorer09 I pulled in your patches (took me a bit to compare with what I already had). LGTM.

I noticed a small issue with the LED mode of the Battery meter: When you switch from LED mode to Text mode, you see the LED mode text until the next refresh cycle happened. To resolve this, we could:

a) just refresh rendering when a mode is changed
b) ignore this glitched display

Anyway: The LED mode of the battery meter looks a bit cramped (XX.X%(A/C)); should better be XX.X% (A/C). We could maybe opt to also make this format the normal Text mode display (eliminating the previous issue in one go).

[Explorer09]

I noticed a small issue with the LED mode of the Battery meter: When you switch from LED mode to Text mode, you see the LED mode text until the next refresh cycle happened. To resolve this, we could:

a) just refresh rendering when a mode is changed b) ignore this glitched display

I think this should be a separate issue. I would vote for a force refresh when a mode is changed (a).

Anyway: The LED mode of the battery meter looks a bit cramped (XX.X%(A/C)); should better be XX.X% (A/C). We could maybe opt to also make this format the normal Text mode display (eliminating the previous issue in one go).

I would choose to make the space also in text mode. (There's no reason against that).

[Explorer09]

@BenBE By the way. I feel little uncomfortable when we allow a supportedModes field to be zero. If possible, I would like all meters to have explicit supportedModes. This would eliminate the runtime code that falls back to METERMODE_DEFAULT_SUPPORTED, potentially saving some code size.

[BenBE]

I noticed a small issue with the LED mode of the Battery meter: When you switch from LED mode to Text mode, you see the LED mode text until the next refresh cycle happened. To resolve this, we could:
a) just refresh rendering when a mode is changed b) ignore this glitched display

I think this should be a separate issue. I would vote for a force refresh when a mode is changed (a).

Okay, best idea where to place this?

Anyway: The LED mode of the battery meter looks a bit cramped (XX.X%(A/C)); should better be XX.X% (A/C). We could maybe opt to also make this format the normal Text mode display (eliminating the previous issue in one go).

I would choose to make the space also in text mode. (There's no reason against that).

The space is there in Text mode, but missing in LED mode.

Text mode: XX.X% (Running on A/C)
LED mode: XX.X%(A/C)

[BenBE]

@BenBE By the way. I feel little uncomfortable when we allow a supportedModes field to be zero. If possible, I would like all meters to have explicit supportedModes. This would eliminate the runtime code that falls back to METERMODE_DEFAULT_SUPPORTED, potentially saving some code size.

We could assert this to be non-zero. Wouldn't even argue about code size re this …

[Explorer09]

I noticed a small issue with the LED mode of the Battery meter: When you switch from LED mode to Text mode, you see the LED mode text until the next refresh cycle happened. To resolve this, we could:
a) just refresh rendering when a mode is changed b) ignore this glitched display

I think this should be a separate issue. I would vote for a force refresh when a mode is changed (a).

Okay, best idea where to place this?

When a meter mode in changed in the UI, that is MetersPanel_eventHandler. I remember it's only a boolean flag to force refresh, is it not?

The space is there in Text mode, but missing in LED mode.

Text mode: XX.X% (Running on A/C)
LED mode: XX.X%(A/C)

The lack of space is probably due to bar drawing.
If we insert a space there, the battery meter text would look like 100.0%|(A/C) when drawn with a bar and the battery is full.

Considering CPU meters already use spaces to separate frequency and temperature fields, it won't look visually inconsistent if we insert a space for Battery meter as well.

[BenBE]

I noticed a small issue with the LED mode of the Battery meter: When you switch from LED mode to Text mode, you see the LED mode text until the next refresh cycle happened. To resolve this, we could:
a) just refresh rendering when a mode is changed b) ignore this glitched display

I think this should be a separate issue. I would vote for a force refresh when a mode is changed (a).

Okay, best idea where to place this?

When a meter mode in changed in the UI, that is MetersPanel_eventHandler. I remember it's only a boolean flag to force refresh, is it not?

It's a matter of the displayed text being cached and still wrong. There is a redraw done already.

The space is there in Text mode, but missing in LED mode.

Text mode: XX.X% (Running on A/C)
LED mode: XX.X%(A/C)

The lack of space is probably due to bar drawing. If we insert a space there, the battery meter text would look like 100.0%|(A/C) when drawn with a bar and the battery is full.

Considering CPU meters already use spaces to separate frequency and temperature fields, it won't look visually inconsistent if we insert a space for Battery meter as well.

ACK.

[Explorer09]

It's a matter of the displayed text being cached and still wrong. There is a redraw done already.

Or maybe call Meter.draw() to force updating the cache? I don't know.
(Call meter->draw() in MetersPanel_eventHandler)

[Explorer09]

Please pull in these two patches to your PR:
Explorer09@252c882 "Restrict supported modes for (more) text-only meters"
Explorer09@98f0ce3 "Explicitly specify supported modes of all meters"

[BenBE]

@Explorer09 Anything else to do in your opinion? Otherwise I'd go ahead an merge this one soon …

[Explorer09]

Anything else to do in your opinion? Otherwise I'd go ahead an merge this one soon …

Nothing except that 9f315bb and 252c882 may be squashed together during merging.