adding bar_label parameter to htoprc #1546

rustedusted · 2024-10-03T19:46:25Z

adding bar_label feature to change the pipe in htop to any other ascii character user wants

this is addressing feature request #647

The PR only has support for normal ascii characters

i can extend this feature to add UTF-8 characters but the CONTRIBUTING.md suggested that newer features shouldn't unnecessarily increase the footprint of the program
(this is my first commit btw so i don't know much)

also Please look if the change in the file Settings.c is correct
options[1][0] doesn't seem a good way to get the character on right of = sign

rustedusted · 2024-10-03T19:46:50Z

i will right away commit all the things that might be necessary

BenBE · 2024-10-03T20:57:25Z

While the PR itself looks fine from a code PoV adding new settings is always a bit tricky as future backwards compatibility has to be taken into account. In the mentioned issue, where this new feature is somewhat discussed, the aim was more for something extensible, that may be added upon later if necessary.

As the PR looks now, this is kinda fixed just on this one usecase of just a single character and somewhat neglects the extensibility for e.g. "sub-pixel" highlighting also discussed in the linked issue.

Another aspect is that all settings should be available in the UI. This is currently not the case with this PR.

@fasterit @natoscott : Further thoughts?

rustedusted · 2024-10-04T03:41:23Z

Oh thanks for the input, :D
I can add sub pixel rendering

maybe an integer called bar_type={1-6} where each number represents the type of bar to use

is this fine or do you guys have any other ideas?

also you said the settings should be available in UI
should I add a shortcut key for it?? or when the bar is clicked the pipe cycles through different character-set?

BenBE · 2024-10-04T09:00:15Z

I think for the UI settings it should suffice to have it selectable in the setup screen using F2 similar to how the Hide main function bar setting is handled. Have a look at how number inputs for the settings are done; you can do a new one for the style. This setting can, as you suggested, be an integer in the config file.

Possible future extension: If we really wanted to include a custom option (fully hypothetical), that one could add 18 code points (9 for each bar level) after the initial "custom" designator. But please leave this out for now.

rustedusted · 2024-10-05T05:54:51Z

oh will get into that right away

Explorer09 · 2024-10-05T23:42:18Z

@rustedusted
There are two main things to address regarding the whole issue of Bar meter characters:

The "subpixel" rendering. (Technically it's character cell not pixel, but anyway.) It would require an algorithm rewrite, and while I had some ideas on the algorithm (because I wrote the wonderful Dynamic scaling & Graph meter coloring (new) #714 feature that's still waiting to be merged), I am still unsure about how useful this "subpixel" rendering of bars can provide comparing to the code complexity that will add. (Also, once implemented, the set of characters to be used would likely be hard-coded, U+2588 - U+258F, as customization on that is rarely useful.)
The set of characters to be used in a monochrome terminal. It is defined in BarMeterMode_characters. If we want customizability, it should be customizing that set instead of just one character. But still, I can't see how that customization can be useful.

BenBE · 2024-10-06T09:29:17Z

@rustedusted There are two main things to address regarding the whole issue of Bar meter characters:

The "subpixel" rendering. (Technically it's character cell not pixel, but anyway.) It would require an algorithm rewrite, and while I had some ideas on the algorithm (because I wrote the wonderful Dynamic scaling & Graph meter coloring (new) #714 feature that's still waiting to be merged), I am still unsure about how useful this "subpixel" rendering of bars can provide comparing to the code complexity that will add. (Also, once implemented, the set of characters to be used would likely be hard-coded, U+2588 - U+258F, as customization on that is rarely useful.)

From a technical PoV the complexity is basically just: Treat the bar meter as having 8 times more space and use the remainder when dividing by 8 as the index into the character table.

One reason for #714 not being merged yet is the heap of complexity it introduces despite what it tries to achieve. That's also why I pushed the "per meter" flags for rendering, as this allows different meter renderings to be used where necessary, thus reducing the complexity in the meter rendering (only has to care that its exact rendering works properly for the data provided).

The set of characters to be used in a monochrome terminal. It is defined in BarMeterMode_characters. If we want customizability, it should be customizing that set instead of just one character. But still, I can't see how that customization can be useful.

That set of characters is meant for a different feature and should be handled as such. That other feature is not subject of this PR. Please don't mix up different things.

rustedusted · 2024-10-06T09:54:08Z

Hey there sorry for the delay was getting myself acquainted with codebase so i could solve take on more issues

added the sub pixel rendering, however as you might see the bars are distancing themselves between two differently colored bars

should i add sub pixel only for the last bar and keep other bars non subpixel so that it seems consistent instead of disjoint??
or should i make the background of the ending characters of bars to be the foreground color of next bar?

if you have any other idea please let me know i feel like the next commits will be much faster now that i know significantly more about the code ig 😄

DisplayOptionsPanel.c

BenBE · 2024-10-06T10:06:19Z

Meter.c

+      const char* bars[7][8] = {
+         {"|","|","|","|","|","|","|","|"},
+         {"#","#","#","#","#","#","#","#"},
+         {"⡀","⡄","⡆","⡇","⣇","⣧","⣷","⣿"},
+         {"░","░","▒","▒","▓","▓","█","█"},
+         {"▏","▎","▍","▌","▋","▊","▉","█"},
+         {"▁","▂","▃","▄","▅","▆","▇","█"},
+         {"▌","▌","▌","▌","█","█","█","█"}
+      };


Include the whitespace character for the various sets, as for e.g. braile you want to use the "zero dots" codepoint, instead of the normal blank character for consistency.

Also you can use " ||||||||", which while using one byte more is more compact to write. Furthermore, your type should be:

Suggested change

const char* bars[7][8] = {

{"|","|","|","|","|","|","|","|"},

{"#","#","#","#","#","#","#","#"},

{"⡀","⡄","⡆","⡇","⣇","⣧","⣷","⣿"},

{"░","░","▒","▒","▓","▓","█","█"},

{"▏","▎","▍","▌","▋","▊","▉","█"},

{"▁","▂","▃","▄","▅","▆","▇","█"},

{"▌","▌","▌","▌","█","█","█","█"}

};

static const char* bars[7] = {

" ||||||||",

" ########",

" ⡀⡄⡆⡇⣇⣧⣷⣿",

" ░░▒▒▓▓██",

" ▏▎▍▌▋▊▉█",

" ▁▂▃▄▅▆▇█",

" ▌▌▌▌████",

};

(Used blank for all of them for now, please update the blanks for the graphical ones)

Will commit everything bundled in one commit

i'll change the data type from instead of char to wchar so that we won't have to worry about converting char to wchar on everytime requested

thanks for the changes i was quite uncertain while writing this :D

also i had to ask why don't we use normal space ?
and which is the utf8 character that you're referring to instead of the space.

For Braille you can use this one:
https://www.compart.com/en/unicode/U+2800

Reason for using other ones is for consistency, because sometimes the spacehas a different width in fonts compared to the default blank …

@BenBE what do you think about this ??

should i add it ?
if yes,
should i replace it instead of the currently used characters for braille?

or should we add utf16 later ??

btw adding utf16 is as easy as just adding new line in the array

I think, Unicode 13 (March 2020) is fine. For Unicode 16 (September 2024) we can add this as a commented out inclusion, but should not compile it in by default (people shall edit their code if they wish to use it). Regarding the Unicode 16 variant I also have my doubts, if this set should be abbreviated or if there is a slightly extended version. Also for braille you could even do 0 to 8 dots starting at the bottom and adding the bottom-left most empty dot.

P.S.: There's a tool called charmap that can be useful to look up these characters.

so is this done??

if there is anything yet to add please do let me know

Meter.c

rustedusted · 2024-10-07T13:25:56Z

i have added everything in the following commits that was suggested

if there is anything more please let me know

BenBE

Please rebase your PR on top of main instead of merging. Also fixes to existing commits should be squashed into the appropriate commits instead of cluttering the history with trial and error: The commit history of a PR should show, what needs to be done to implement a feature, not every turn you took on the way to get there.

Adding the braces can (and should) stay a separate commit, as it only does code style changes and doesn't touch any functionality.

BenBE · 2024-10-07T18:45:26Z

Meter.c

@@ -124,26 +125,50 @@ static void BarMeterMode_draw(Meter* this, int x, int y, int w) {

   // First draw in the bar[] buffer...
   int offset = 0;
+   double actualBarWidth=0;


You should be able to avoid this floating point code by defining int wsub = w * barsLen; (rearranging code may apply) and using wsub instead of w whenever referring to the length of the bar, instead of actual cell position on the screen.

hey there does this mean i make int actualBarWidth=0;

and no other change??

or do i change entire code to use wsub instead of w (width)?

BenBE · 2024-10-07T18:46:00Z

Meter.c

   for (uint8_t i = 0; i < this->curItems; i++) {
      double value = this->values[i];
      if (isPositive(value) && this->total > 0.0) {
         value = MINIMUM(value, this->total);
-         blockSizes[i] = ceil((value / this->total) * w);
+         actualBarWidth = ((value / this->total) * w);


The outer parens aren't necessary and should be removed.

BenBE · 2024-10-07T18:47:58Z

Meter.c

+
+      const wchar_t *barChars = &bars[settings->barType][1];
+      int barsLen = wcslen(barChars);
+      int subPixel = floor(actualBarWidth * barsLen);


See comment above. Will most likely reduce to a no-op in that context because no further scaling is needed.

rustedusted · 2024-10-08T03:24:37Z

Oh hey there you're right about squashing I didn't know a lot about git just learned few things will make a new pull request to do this

I'm really very sorry for the inconvenience

Explorer09 · 2024-10-08T03:28:47Z

@rustedusted What are you doing? Try git rebase with -i option and it can help you with many things. No need to make another pull request.

rustedusted · 2024-10-08T03:32:50Z

@Explorer09 i was trying to merge main i did not create a new branch

rustedusted · 2024-10-08T03:33:29Z

in fact i didn't know i had to create a new branch for that matter
this was my first commit tbh

is there a way to recover this?!

rustedusted · 2024-10-08T03:40:28Z

this is the entire history of commits all the commits should be squashed ig

Explorer09 · 2024-10-08T03:40:58Z

@rustedusted I don't recommend breaking existing discussions so I would suggest you to keep this branch main for now unless you really want the PR to be closed.

You can set up a new branch upstream-main (using git branch command) and have upstream-main follow the main of this repository, and you won't get conflicts.

rustedusted · 2024-10-09T19:41:55Z

hey there just an update i'm working on this issue couldn't work on this yesterday will definitely try putting a commit tomorrow

rustedusted · 2024-10-09T19:42:04Z

organised commits ig:D

rustedusted · 2024-10-10T12:45:58Z

i've made a branch with appropriate changes named new-stream

do i make a new pull request??
@Explorer09

rustedusted · 2024-10-10T12:50:32Z

hey also wanted to try out solving other issues can i take a new issue as i feel like this one is about to be solved ig idk tbh:|

BenBE · 2024-10-11T18:19:50Z

do i make a new pull request??

No, just force-push your updated PR on the branch you used to make this one. No need for a new PR, as this splits the discussion unnecessarily.

rustedusted · 2024-10-14T18:22:46Z

oh hey there very sorry i did not notice the warning i thought we were compiling with Werror and Wall flag

rustedusted · 2024-10-14T18:37:00Z

done i don't think there should be any warnings now also i saw the snippet you sent

cc1: all warnings being treated as errors

this was a message in the snippet you sent i don't get such messages in fact it just showed warnings and ran without any issue

BenBE · 2024-10-14T18:54:14Z

See:

htop/.github/workflows/ci.yml

Lines 17 to 29 in 4102862

    
               - name: Bootstrap 
        
                 run: ./autogen.sh 
        
               - name: Configure 
        
                 run: ./configure --enable-werror --enable-affinity --disable-unicode --disable-sensors || (cat config.log; exit 1) 
        
               - name: Enable compatibility modes 
        
                 run: | 
        
                   sed -i 's/#define HAVE_FSTATAT 1/#undef HAVE_FSTATAT/g' config.h 
        
                   sed -i 's/#define HAVE_OPENAT 1/#undef HAVE_OPENAT/g' config.h 
        
                   sed -i 's/#define HAVE_READLINKAT 1/#undef HAVE_READLINKAT/g' config.h 
        
               - name: Build 
        
                 run: make -k 
        
               - name: Distcheck 
        
                 run: make distcheck DISTCHECK_CONFIGURE_FLAGS="--enable-werror --enable-affinity --disable-unicode --disable-sensors"

Pass --enable-werror to ./configure …

Meter.c

rustedusted · 2024-10-15T03:24:55Z

all suggested changes have been made :D

BenBE · 2024-10-15T07:23:18Z

And some of the updates I did were dropped …

diff --git a/Meter.c b/Meter.c
index 00311a57..21e2afd4 100644
--- a/Meter.c
+++ b/Meter.c
@@ -132,14 +132,15 @@ static void BarMeterMode_draw(Meter* this, int x, int y, int w) {
    assert(startPos <= w);
    assert(startPos + w <= RichString_sizeVal(bar));
 
-   const Settings* settings = this->host->settings;
    int blockSizes[10];
 
    // First draw in the bar[] buffer...
    int offset = 0;
+
+   const Settings* settings = this->host->settings;
    const wchar_t* barChars = &bars[settings->barType][1];
    const size_t barLen = wcslen(barChars);
-   const uint8_t wsub = w * barLen;
+   const size_t wsub = w * barLen;
 
    for (uint8_t i = 0; i < this->curItems; i++) {
       double value = this->values[i];
@@ -150,6 +151,7 @@ static void BarMeterMode_draw(Meter* this, int x, int y, int w) {
          blockSizes[i] = 0;
          continue;
       }
+
       if (isPositive(value) && this->total > 0.0) {
          value = MINIMUM(value, this->total);
          actualWidth = ceil((value / this->total) * wsub);
@@ -161,7 +163,7 @@ static void BarMeterMode_draw(Meter* this, int x, int y, int w) {
       // (Control against invalid values)
       nextOffset = CLAMP(nextOffset, 0, w);
 
-      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) {
                assert(i < strlen(BarMeterMode_characters));
@@ -174,7 +176,7 @@ static void BarMeterMode_draw(Meter* this, int x, int y, int w) {
          }
       }
 
-      RichString_setChar(&bar, startPos + nextOffset-1, barChars[actualWidth % barLen]);
+      RichString_setChar(&bar, startPos + nextOffset - 1, barChars[actualWidth % barLen]);
 
       offset = nextOffset;
    }

rustedusted · 2024-10-15T17:34:25Z

oh sorry did not notice that i guess the changes are made now but

this is what i get

rustedusted · 2024-10-15T17:34:52Z

is there anything remaining

rustedusted · 2024-10-15T17:55:04Z

it was division with zero solved that issue :D was slightly confused for some time

rustedusted · 2024-10-15T17:56:01Z

solved all the checks

rustedusted · 2024-10-15T18:27:12Z

now i don't know what can go wrong

rustedusted · 2024-10-15T18:27:28Z

if there's anything do let me know :D

BenBE · 2024-10-16T08:26:08Z

If you reword that comparison you can avoid the division completely … Although I'm not fully sure if that short-hand is needed.

rustedusted · 2024-10-16T12:38:05Z

oh are you referring to line 157 in Meter.c

well the thing is that sometimes value is extremely small so just to avoid a single level it felt correct

most times the value is extremely small so it just occupies a single character space for a character containing mostly empty space something like ⡀
in braille

you can try removing it and if you feel that is correct then we can use it

BenBE · 2024-10-17T07:43:58Z

Meter.c

         blockSizes[i] = ceil((value / this->total) * w);
      } else {
         blockSizes[i] = 0;
      }
+
+      if ((value / this->total) * wsub < 0.5) {


Avoid division by zero …

Suggested change

if ((value / this->total) * wsub < 0.5) {

if (value * wsub < 0.5 * this->total) {

oh right sorry for that made the same mistake

oh wait i think making just one if condition is possible

see the new changes and see if they seem fine

because we need to continue even when total is zero

utf8 bars can be used by going into setup(F2) These bars enable subpixel rendering

rustedusted · 2024-10-17T13:06:11Z

it works as expected in this as well removing if might actually be a good method

rustedusted · 2024-10-17T13:06:19Z

i don't know though i'm a newbie

rustedusted · 2024-10-20T06:57:26Z

hey there any updates to this ?!

BenBE added needs-discussion 🤔 Changes need to be discussed and require consent new feature Completely new feature labels Oct 3, 2024

BenBE reviewed Oct 6, 2024

View reviewed changes

BenBE reviewed Oct 7, 2024

View reviewed changes

Meter.c Outdated Show resolved Hide resolved

BenBE reviewed Oct 7, 2024

View reviewed changes

Meter.c Outdated Show resolved Hide resolved

BenBE requested changes Oct 7, 2024

View reviewed changes

rustedusted closed this Oct 8, 2024

rustedusted reopened this Oct 8, 2024

rustedusted force-pushed the main branch 2 times, most recently from faa6f53 to 791d28e Compare October 12, 2024 05:26

rustedusted force-pushed the main branch 2 times, most recently from a3d603c to 546915b Compare October 14, 2024 18:33

Explorer09 reviewed Oct 14, 2024

View reviewed changes

Meter.c Outdated Show resolved Hide resolved

Explorer09 reviewed Oct 14, 2024

View reviewed changes

Meter.c Outdated Show resolved Hide resolved

Explorer09 reviewed Oct 14, 2024

View reviewed changes

Meter.c Outdated Show resolved Hide resolved

Explorer09 reviewed Oct 14, 2024

View reviewed changes

Meter.c Outdated Show resolved Hide resolved

Explorer09 reviewed Oct 14, 2024

View reviewed changes

Meter.c Outdated Show resolved Hide resolved

rustedusted force-pushed the main branch from 15cf7ab to 7ebc8e0 Compare October 15, 2024 03:18

BenBE force-pushed the main branch from 7ebc8e0 to 67eb2c9 Compare October 15, 2024 07:23

rustedusted force-pushed the main branch from 67eb2c9 to db8ea97 Compare October 15, 2024 17:54

rustedusted force-pushed the main branch from db8ea97 to 1218172 Compare October 15, 2024 18:23

BenBE reviewed Oct 17, 2024

View reviewed changes

adding different types of bars

5ffb9e6

utf8 bars can be used by going into setup(F2) These bars enable subpixel rendering

rustedusted force-pushed the main branch from 1218172 to 5ffb9e6 Compare October 17, 2024 13:05

	if ((value / this->total) * wsub < 0.5) {
	if (value * wsub < 0.5 * this->total) {

adding bar_label parameter to htoprc #1546

Are you sure you want to change the base?

adding bar_label parameter to htoprc #1546

Conversation

rustedusted commented Oct 3, 2024

rustedusted commented Oct 3, 2024

BenBE commented Oct 3, 2024

rustedusted commented Oct 4, 2024

BenBE commented Oct 4, 2024

rustedusted commented Oct 5, 2024

Explorer09 commented Oct 5, 2024

BenBE commented Oct 6, 2024

rustedusted commented Oct 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustedusted commented Oct 7, 2024

BenBE left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustedusted commented Oct 8, 2024

Explorer09 commented Oct 8, 2024

rustedusted commented Oct 8, 2024

rustedusted commented Oct 8, 2024

rustedusted commented Oct 8, 2024

Explorer09 commented Oct 8, 2024

rustedusted commented Oct 9, 2024

rustedusted commented Oct 9, 2024

rustedusted commented Oct 10, 2024

rustedusted commented Oct 10, 2024

BenBE commented Oct 11, 2024

rustedusted commented Oct 14, 2024

rustedusted commented Oct 14, 2024

BenBE commented Oct 14, 2024

rustedusted commented Oct 15, 2024

BenBE commented Oct 15, 2024

rustedusted commented Oct 15, 2024

rustedusted commented Oct 15, 2024

rustedusted commented Oct 15, 2024

rustedusted commented Oct 15, 2024

rustedusted commented Oct 15, 2024

rustedusted commented Oct 15, 2024

BenBE commented Oct 16, 2024

rustedusted commented Oct 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustedusted commented Oct 17, 2024

rustedusted commented Oct 17, 2024

rustedusted commented Oct 20, 2024