-
-
Notifications
You must be signed in to change notification settings - Fork 541
Implement different display types for bar meters #1794
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
base: main
Are you sure you want to change the base?
Conversation
|
referencing with issue #647 |
|
One more thing the function didn't have any if statement in it so felt awkward to do that |
|
[x] Resolve the conflict marker in the latest commit(not using the previous commit) |
584ccc7 to
a9b7037
Compare
Settings.c
Outdated
| this->hideFunctionBar = atoi(option[1]); | ||
| #ifdef HAVE_LIBNCURSESW | ||
| } else if (String_eq(option[0], "bar_type")) { | ||
| this->barType = atoi(option[1]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value read here isn't protected against an invalid configuration file. Please ensure, that the value in this->barType is sanitized to be a valid bar type index.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be a good idea to set this->barType to 0 whenever we read an invalid value from the config. This makes the field forward-compatible to any future extensions of the list. Also, we should use strtol for parsing this field to get a defined behavior on integer overflow.
Unfortunately we don't have a macro to easily produce code like this one:
{
long value = strtol(option[1], NULL, 10);
if (value < 0 || value > BAR_METER_NUM_STYLES)
value = 0;
this->barType = (unsigned int)value;
}| blockSizes[i] = ceil((value / this->total) * w); | ||
| blockSizes[i] = MINIMUM(blockSizes[i], w - offset); | ||
|
|
||
| #ifdef HAVE_LIBNCURSESW | ||
| extraWidth = (int)ceil((value / this->total) * w * barLen) % barLen; | ||
| #endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs some minor update. The extraWidth calculation probably should re-use the value calculated in the blockSizes[i] assignments above.
Meter.c
Outdated
| L"||||||||", | ||
| L"########", | ||
| L"⣿⡀⡄⡆⡇⣇⣧⣷", | ||
| L"█░░▒▒▓▓█", | ||
| L"█▏▎▍▌▋▊▉", | ||
| L"█▁▂▃▄▅▆▇", | ||
| L"█▌▌▌▌███", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having the full block final seems more natural. Any reason to put it first and skip the per-profile blank instead?
DisplayOptionsPanel.c
Outdated
| Panel_add(super, (Object*) NumberItem_newByRef("Hide main function bar (0 - off, 1 - on ESC until next input, 2 - permanently)", &(settings->hideFunctionBar), 0, 0, 2)); | ||
|
|
||
| #ifdef HAVE_LIBNCURSESW | ||
| Panel_add(super, (Object*) NumberItem_newByRef("Bar Type (0-6)", &(settings->barType), 0, 0, 6)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe 0 = default to avoid having this needing to updated whenever there's a new character theme.
Settings.h
Outdated
| #endif | ||
| int hideFunctionBar; // 0 - off, 1 - on ESC until next input, 2 - permanently | ||
| #ifdef HAVE_LIBNCURSESW | ||
| int barType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to use unsigned int for this field?
| assert(settings->barType < ( sizeof(bars) / sizeof(wchar_t*) )); | ||
| assert(settings->barType >= 0); | ||
| const wchar_t* currBar = bars[settings->barType]; | ||
| int barLen = (int)wcslen(currBar); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I consider this function call redundant when you have declared the list of bars to have equal length.
Not sure if this is what you intended, but I think the bar meter characters can be defined with variable widths if you wish.
Like this:
static const wchar_t* BarMeterMode_drawStyles[] = {
L"|",
L"#",
L"⡀⡄⡆⡇⣇⣧⣷⣿",
L"░▒▓█",
L"▏▎▍▌▋▊▉█",
L"▁▂▃▄▅▆▇█",
L"▌█",
};There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bars currently have all the same length, but the intention is to actually allow for bars to contain different number of symbols.
Meter.c
Outdated
| #include <stdio.h> | ||
| #include <stdlib.h> | ||
| #include <string.h> | ||
| #include <wchar.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this include line. wchar.h is better included indirectly via ProvideCurses.h.
Meter.c
Outdated
| #include <float.h> | ||
| #include <limits.h> // IWYU pragma: keep | ||
| #include <math.h> | ||
| #include <stdio.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see any stdio.h function being used at all in this PR.
|
Please squash/fixup changes into the original commit by rebasing. Details are in the style guide. |
39d372a to
5f1a7a9
Compare
Allow bar meters to enable "sub-pixel" rendering. Bar meter styles can be changed in setup (F2) under Display Options. Co-Authored-By: Benny Baumann <[email protected]>
5f1a7a9 to
e739352
Compare
|
here's the latest commit that contains all the suggested things |
| assert(settings->barType < ( sizeof(bars) / sizeof(wchar_t*) )); | ||
| assert(settings->barType >= 0); | ||
| const wchar_t* currBar = bars[settings->barType]; | ||
| int barLen = (int)wcslen(currBar); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bars currently have all the same length, but the intention is to actually allow for bars to contain different number of symbols.
| (1 << LED_METERMODE) | \ | ||
| 0) // Avoids edits when updating | ||
|
|
||
| #define BAR_METER_NUM_STYLES 7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| #define BAR_METER_NUM_STYLES 7 | |
| extern const size_t bar_meter_num_styles; | |
And below the definition of bars' for the meter styles in Meter.c`:
const size_t bar_meter_num_styles = sizeof(bars) / sizeof(*bars);This avoids an additional point of manual updating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BenBE This has the problem that when compiling Setting.c, compiler cannot inline the constant but has to refer to the external variable for the number. In other words, the compiled code would be sub-optimal.
I think the better approach is to move the definitions of the bar styles into the header. Look at the unitPrefixes definition in XUtils.h for what I mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm aware of this downside, but AFAICT there are no real places where looking up this constant happens inside a tight loop …
Meter.c
Outdated
| } else { | ||
| } | ||
| #ifdef HAVE_LIBNCURSESW | ||
| else if(CRT_utf8 && settings->barType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole block of else if (CRT_utf8 && settings->barType) { ... } can merge with the else { ... } block below so we can reduce duplicate code.
| #endif | ||
| int hideFunctionBar; // 0 - off, 1 - on ESC until next input, 2 - permanently | ||
| #ifdef HAVE_LIBNCURSESW | ||
| unsigned int barType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the proposed bar drawing styles has | (vertical bar, the default), and # (number sign), both can be drawn in ASCII-only mode, I think we can remove the #ifdef HAVE_LIBNCURSESW conditional and allow the setting to be available in both ASCII-only build and Unicode build of htop.
Note: only the first two drawing styles can be selected in ASCII. The rest can be guarded in the #ifdef HAVE_LIBNCURSESW so they require Unicode to enable:
#ifdef HAVE_LIBNCURSESW
static const wchar_t* barMeterDrawStyles[] = {
L"|",
L"#",
L"⡀⡄⡆⡇⣇⣧⣷⣿",
L"░▒▓█",
L"▏▎▍▌▋▊▉█",
L"▁▂▃▄▅▆▇█",
L"▌█",
};
#else
static const char* barMeterDrawStyles[] = {
"|",
"#"
};
#endif
static const size_t numBarMeterStyles = ARRAYSIZE(barMeterDrawStyles);| for (int j = offset; j < nextOffset; j++) | ||
| for (int j = offset; j < nextOffset; j++) { | ||
| if (RichString_getCharVal(bar, startPos + j) == ' ') { | ||
| if (CRT_colorScheme == COLORSCHEME_MONOCHROME) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is one thing I wish to add when the feature of changing bar drawing character is added. It's this:
| if (CRT_colorScheme == COLORSCHEME_MONOCHROME) { | |
| if (CRT_colorScheme == COLORSCHEME_MONOCHROME && Meter_maxItems(this) != 1) { |
Make the bar character changeable for meters with only one item.
--- a/Meter.h
+++ b/Meter.h
@@ -100,6 +100,7 @@ typedef struct MeterClass_ {
#define Meter_attributes(this_) As_Meter(this_)->attributes
#define Meter_name(this_) As_Meter(this_)->name
#define Meter_uiName(this_) As_Meter(this_)->uiName
+#define Meter_maxItems(this_) As_Meter(this_)->maxItems
#define Meter_isMultiColumn(this_) As_Meter(this_)->isMultiColumn
#define Meter_isPercentChart(this_) As_Meter(this_)->isPercentChart
Co-authored-by: BenBE <[email protected]>
Co-authored-by: BenBE <[email protected]>
Co-authored-by: BenBE <[email protected]>
Co-authored-by: BenBE <[email protected]>
Allow bar meters to enable "sub-pixel" rendering.
Bar meter styles can be changed in setup (F2) under Display Options.
Co-Authored-By: Benny Baumann [email protected]