Skip to content
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

Migrate away from legacy ncurses functions attrset, attron,attroff, ... #1541

Open
C0rn3j opened this issue Sep 28, 2024 · 6 comments
Open
Labels
code quality ♻️ Code quality enhancement dependencies Pull requests that update a dependency file enhancement Extension or improvement to existing feature

Comments

@C0rn3j
Copy link
Contributor

C0rn3j commented Sep 28, 2024

Ran into this while trying to fix #243

Fixing these will allow htop to correctly handle the 65k available colors and the necessary version bump enables 8-bit RGB colors (24-bit).

Maybe migrate to wide chars where doable -> wattr_on/wattr_off/wattr_set?
In which case other things also have to be migrated, see the docs.

Here's picmap source from ncurses test examples - https://github.com/ThomasDickey/ncurses-snapshots/blob/master/test/picsmap.c

With the 6.1+ bump:

init_pair -> init_extended_pair

% grep -R init_pair 2>/dev/null
test_spec.lua:            curses.init_pair(pair, foreground, background)
CRT.c:            init_pair(ColorIndex(i, j), i, bg);
CRT.c:   init_pair(ColorIndexGrayBlack, grayBlackFg, grayBlackBg);
CRT.c:   init_pair(ColorIndexWhiteDefault, White, -1);

attron -> attr_on

NEW:   attr_on(resetColor, NULL);

% grep -R attron 2>/dev/null
CRT.c:   attron(resetColor);

attroff -> attr_off

NEW:   attr_off(resetColor, NULL);

% grep -R attroff 2>/dev/null
CRT.c:   attroff(resetColor);

attrset -> attr_set

% grep -R attrset 2>/dev/null
Action.c:   attrset(attr);
Action.c:   attrset(CRT_colors[HELP_BOLD]);
Action.c:   attrset(CRT_colors[DEFAULT_COLOR]);
Action.c:   attrset(CRT_colors[DEFAULT_COLOR]);
Action.c:   attrset(CRT_colors[DEFAULT_COLOR]);
Action.c:   attrset(CRT_colors[DEFAULT_COLOR]);
Action.c:   attrset(CRT_colors[DEFAULT_COLOR]);
Action.c:      attrset((helpLeft[item].roInactive && readonly) ? CRT_colors[HELP_SHADOW] : CRT_colors[DEFAULT_COLOR]);
Action.c:      attrset((helpLeft[item].roInactive && readonly) ? CRT_colors[HELP_SHADOW] : CRT_colors[HELP_BOLD]);
Action.c:         attrset((helpLeft[item].roInactive && readonly) ? CRT_colors[HELP_SHADOW] : CRT_colors[PROCESS_THREAD]);
Action.c:         attrset((helpLeft[item].roInactive && readonly) ? CRT_colors[HELP_SHADOW] : CRT_colors[PROCESS_THREAD]);
Action.c:      attrset((helpRight[item].roInactive && readonly) ? CRT_colors[HELP_SHADOW] : CRT_colors[HELP_BOLD]);
Action.c:      attrset((helpRight[item].roInactive && readonly) ? CRT_colors[HELP_SHADOW] : CRT_colors[DEFAULT_COLOR]);
Action.c:   attrset(CRT_colors[HELP_BOLD]);
Action.c:   attrset(CRT_colors[DEFAULT_COLOR]);
Meter.c:   attrset(CRT_colors[METER_TEXT]);
Meter.c:   attrset(CRT_colors[RESET_COLOR]);
Meter.c:   attrset(CRT_colors[METER_TEXT]);
Meter.c:   attrset(CRT_colors[BAR_BORDER]);
Meter.c:   attrset(CRT_colors[RESET_COLOR]);
Meter.c:   attrset(CRT_colors[RESET_COLOR]);
Meter.c:   attrset(CRT_colors[METER_TEXT]);
Meter.c:         attrset(CRT_colors[colorIdx]);
Meter.c:   attrset(CRT_colors[RESET_COLOR]);
Meter.c:   attrset(CRT_colors[LED_COLOR]);
Meter.c:         const cchar_t wc = { .chars = { c, '\0' }, .attr = 0 }; /* use LED_COLOR from attrset() */
Meter.c:   attrset(CRT_colors[RESET_COLOR]);
InfoScreen.c:   attrset(CRT_colors[METER_TEXT]);
InfoScreen.c:   attrset(CRT_colors[DEFAULT_COLOR]);
ScreenManager.c:   attrset(CRT_colors[cur ? SCREENS_CUR_BORDER : SCREENS_OTH_BORDER]);
ScreenManager.c:   attrset(CRT_colors[cur ? SCREENS_CUR_TEXT : SCREENS_OTH_TEXT]);
ScreenManager.c:   attrset(CRT_colors[cur ? SCREENS_CUR_BORDER : SCREENS_OTH_BORDER]);
ScreenManager.c:   attrset(CRT_colors[RESET_COLOR]);
FunctionBar.c:   attrset(CRT_colors[FUNCTION_BAR]);
FunctionBar.c:      attrset(CRT_colors[FUNCTION_KEY]);
FunctionBar.c:      attrset(CRT_colors[FUNCTION_BAR]);
FunctionBar.c:         attrset(CRT_colors[FUNCTION_BAR]);
FunctionBar.c:         attrset(attr);
FunctionBar.c:   attrset(CRT_colors[RESET_COLOR]);
FunctionBar.c:      attrset(CRT_colors[FUNCTION_BAR]);
FunctionBar.c:      attrset(attr);
FunctionBar.c:   attrset(CRT_colors[RESET_COLOR]);
Header.c:   attrset(CRT_colors[RESET_COLOR]);
Panel.c:      attrset(header_attr);
Panel.c:      attrset(CRT_colors[RESET_COLOR]);
Panel.c:            attrset(item.highlightAttr);
Panel.c:            attrset(CRT_colors[RESET_COLOR]);
Panel.c:      attrset(selectionColor);
Panel.c:      attrset(CRT_colors[RESET_COLOR]);

Moreover it looks like htop is written with the 16 (8+8bold) colors of the ancient past in mind, rather than the 256 color standard of yesterday, or the 8-bit RGB of today.

Any arguments about old distributions can rest, as those distributions will simply use old htop, even Ubuntu 20.04 from 2020 has ncurses 6.2... and htop 2.2.0.

@C0rn3j C0rn3j changed the title Migrate away from deprecated ncurses functions attrset, attron and attroff Migrate away from legacy ncurses functions attrset, attron and attroff Sep 28, 2024
@C0rn3j C0rn3j changed the title Migrate away from legacy ncurses functions attrset, attron and attroff Migrate away from legacy ncurses functions attrset, attron,attroff, ... Sep 28, 2024
@BenBE BenBE added enhancement Extension or improvement to existing feature code quality ♻️ Code quality enhancement dependencies Pull requests that update a dependency file labels Sep 29, 2024
@BenBE
Copy link
Member

BenBE commented Sep 29, 2024

If I'm reading your issue correctly, this, together with the comment in #243, would allow for proper preservation of the bold attribute without also introducing brightened-up colors if we were to update the mentioned API calls?

Natural questions that come to mind are:

  1. What is the baseline ncurses version required for this to work?
  2. What features/capabilities does a terminal need to implement, for this to work?
  3. Which common terminals (e.g. qterminal, xterm, Linux text console, rxvt, …) properly support this?
  4. Can the "brightens up on bold" behaviour be detected inside htop to allow automatic fallback to the current behaviour (removing colors for highlighted lines)?
  5. How much influence has the init_color and init_extended_color API and how widely supported is it?
  6. Any sensible suggestions for handling the color schemes already present in htop (to properly support the attributes available with that new API)?

@C0rn3j
Copy link
Contributor Author

C0rn3j commented Sep 29, 2024

this, together with the comment in #243, would allow for proper preservation of the bold attribute without also introducing brightened-up colors if we were to update the mentioned API calls?

The problem in #243 should be able to be tackled without this, however, were that to add more code dependent on the legacy APIs, it would just needlessly be making it more complex to switch away from them later.

  1. What is the baseline ncurses version required for this to work?

init_extended_pair was added to library on 2017.04.01, and released in version 6.1 January 27, 2018

htop currently specifies 6.0 as minimum, defined in 2021 (unless it was required even earlier than this readme commit), that would need to be bumped to 6.1.

  1. What features/capabilities does a terminal need to implement, for this to work?
  2. Which common terminals (e.g. qterminal, xterm, Linux text console, rxvt, …) properly support this?

For this issue for the non-W switchover, that should not matter.

For #243, I do not believe anything new is necessary either, it is mainly a problem with how ncurses handles setting FG+BG color, otherwise we could just edit the original PR #434 to simply only apply the BG color we're passing through and preserve everything else, but ncurses does not work that way.
I did exactly that, just to find out that's not how ncurses works.

For the possible W switchover, which from my understanding is needed for unicode:

  • you have to have ncurses configured to deal with wide characters. For most linux distributions, that means: Your ncurses distribution is based on version 5.4 or later
    • Check
  • you have to have a term program that can display non-ASCII characters.
    • I suppose this would be fine in everything other than ancient terminal emulators, which again, probably won't be running together with modern htop
    • Default tty VTs are not usually capable of rendering Unicode characters, though I am not sure if a W ASCII character would be fine there? Have not tested. I saw htop already ifdefing libncursesw, which is the wide character support library, so this is possibly already being documented/handled in the current codebase... somehow without the W APIs? Shotgunning here.

ncurses|ncursesw is still split on the latest released Debian, so that stilll makes sense with the current codebase that does not hard depend on Wide char support.

  1. Can the "brightens up on bold" behaviour be detected inside htop to allow automatic fallback to the current behaviour (removing colors for highlighted lines)?

Unsure, that sounds like something that'd be great to know, I suppose figuring out why VSC terminal differs from for example Konsole on this front would help reveal if that's possible.

  1. How much influence has the init_color and init_extended_color API and how widely supported is it?

This should be an internal ncurses thing and I don't suppose anything beyond 6.1 + code changes in htop for the newer API would be needed to migrate to it.

At least one problem with the legacy APIs is that they limit ColorPairs to short (36k) instead of int.
While am not sure at which point htop might run into that and other limitations, I can't think of a reason to keep the legacy calls.

  1. Any sensible suggestions for handling the color schemes already present in htop (to properly support the attributes available with that new API)?

Not from me unfortunately, I am not a designer, nor a C/C++ programmer, and my experience with ncurses starts yesterday to boot.

@BenBE
Copy link
Member

BenBE commented Sep 29, 2024

this, together with the comment in #243, would allow for proper preservation of the bold attribute without also introducing brightened-up colors if we were to update the mentioned API calls?

The problem in #243 should be able to be tackled without this, however, were that to add more code dependent on the legacy APIs, it would just needlessly be making it more complex to switch away from them later.

This makes updating the codebase for ncurses 6.1 a reasonable way to go forward.

  1. What is the baseline ncurses version required for this to work?

init_extended_pair was added to library on 2017.04.01, and released in version 6.1 January 27, 2018

htop currently specifies 6.0 as minimum, defined in 2021 (unless it was required even earlier than this readme commit), that would need to be bumped to 6.1.

Given the wide availability of htop even on non-Linux systems, those should be considered to. There should be at least one LTS-style version for each platform that meets that new baseline, though this isn't necessary a hard requirement depending on the platform (no hard feelings for Darwin or Solaris ;-)). If most platforms provide ncurses 6.2 (Feb 2020), 6.3 (Oct 2021) or 6.4 (Dec 2022), even that would be an option, if we already are at increasing the baseline anyway.

  1. What features/capabilities does a terminal need to implement, for this to work?
  2. Which common terminals (e.g. qterminal, xterm, Linux text console, rxvt, …) properly support this?

For this issue for the non-W switchover, that should not matter.

For #243, I do not believe anything new is necessary either, it is mainly a problem with how ncurses handles setting FG+BG color, otherwise we could just edit the original PR #434 to simply only apply the BG color we're passing through and preserve everything else, but ncurses does not work that way. I did exactly that, just to find out that's not how ncurses works.

For the possible W switchover, which from my understanding is needed for unicode:

We will need to keep htop compatible with ncurses versions that do not support Unicode; one example is NetBSD AFAIR.

  • you have to have ncurses configured to deal with wide characters. For most linux distributions, that means: Your ncurses distribution is based on version 5.4 or later

    • Check

Also consider the non-Linux platforms like all the *BSDs.

  • you have to have a term program that can display non-ASCII characters.

    • I suppose this would be fine in everything other than ancient terminal emulators, which again, probably won't be running together with modern htop

Current NetBSD if running their native curses implementation …

  • Default tty VTs are not usually capable of rendering Unicode characters, though I am not sure if a W ASCII character would be fine there? Have not tested. I saw htop already ifdefing libncursesw, which is the wide character support library, so this is possibly already being documented/handled in the current codebase... somehow without the W APIs? Shotgunning here.

The libncursesw related ifdefs serve mostly two purposes:

  1. Signal presence of wide-character support
  2. Presence of the libncursesw library compared to systems that include wide-char support directly in libncurses.

ncurses|ncursesw is still split on the latest released Debian, so that stilll makes sense with the current codebase that does not hard depend on Wide char support.

NetBSD is a bit special there too, cf. this recent issue/PR

  1. Can the "brightens up on bold" behaviour be detected inside htop to allow automatic fallback to the current behaviour (removing colors for highlighted lines)?

Unsure, that sounds like something that'd be great to know, I suppose figuring out why VSC terminal differs from for example Konsole on this front would help reveal if that's possible.

ACK. Also a great plus if we knew on the side of advancing with the issue in #243 that kickstarted this thread.

  1. How much influence has the init_color and init_extended_color API and how widely supported is it?

This should be an internal ncurses thing and I don't suppose anything beyond 6.1 + code changes in htop for the newer API would be needed to migrate to it.

Seems reasonable.

At least one problem with the legacy APIs is that they limit ColorPairs to short (36k) instead of int. While am not sure at which point htop might run into that and other limitations, I can't think of a reason to keep the legacy calls.

I'm quite positive this ain't a problem even right now. ;-)

And while I also don't see much reason to stick to the past for too much, given the existing deployments we shouldn't rush things to far into the future either.

  1. Any sensible suggestions for handling the color schemes already present in htop (to properly support the attributes available with that new API)?

Not from me unfortunately, I am not a designer, nor a C/C++ programmer, and my experience with ncurses starts yesterday to boot.

I was asking because as far as I have skimmed over the manpage links I noticed the new APIs usually take the attributes and colors as two separate arguments, but our existing themes combine these two parameters in one value. There seems to be a macro to split this, but if code can be simplified or made more readable by splitting some of our internal definitions, I think this might be worth a shot.

@C0rn3j
Copy link
Contributor Author

C0rn3j commented Sep 29, 2024

I have thought of a reasonable midway solution to #243 in #1542 with just the attributes.
That will at least get the highlights working for now.

EDIT:

You have to use attr_on or attr_set if you want to use a color pair number >255, 
and you must not use the COLOR_PAIR macro 
(because that will mask the color pair number to 8 bits).

Actually, it looks like the current code is much more limited than just the 35k short values?

EDIT2: Very good read about 24-bit terminal colors which have started being supported in recent years here - mawww/kakoune#1807

That also requires ncurses 6.1 at minimum.

@Explorer09
Copy link
Contributor

OpenBSD curses_attributes(3) man page

So it's safe to assume NetBSD curses supports attr_on, attr_off and attr_set. But I can't say the same for init_extended_pair.

Also it's good to implement the function availability test in the configure script first.

@C0rn3j
Copy link
Contributor Author

C0rn3j commented Oct 9, 2024

What are the benefits of supporting both the limited NetBSD curses backend and the usual ncurses which everything ships, over focusing on ncurses and having full, simple functionality everywhere?

https://github.com/htop-dev/htop/blob/main/netbsd/README.md only describes this situation as NetBSD being one of the last holdouts with their own curses implementation, and htop having support for both.

What I can see from my end is having to write the code twice, in different ways, with caveats, limitations, and having to carefully ifdef everything and having to consider all of that while trying to plumb up the existing codebase or adding new features.

This complex duplication makes it very unappealing to make such contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality ♻️ Code quality enhancement dependencies Pull requests that update a dependency file enhancement Extension or improvement to existing feature
Projects
None yet
Development

No branches or pull requests

3 participants