Ticket: #5020: Allocate fewer ncurses color pairs by egmontkob · Pull Request #5066 · MidnightCommander/mc

egmontkob · 2026-03-13T14:26:06Z

Proposed changes

Reuse ncurses color pairs if possible

Allocating a new ncurses color pair for every new (fg, bg, attrs) tuple might result in running out of available color pairs too soon.

Instead, allocate a new ncurses color pair only if really needed, i.e. if (fg, bg) is a new combination. Apply the attributes separately.

Plus some minor underlying changes.

Resolves: Allocate fewer color pairs (reuse the existing one when only the attributes differ) #5020

Checklist

👉 Our coding style can be found here: https://midnight-commander.org/coding-style/ 👈

I have referenced the issue(s) resolved by this PR (if any)
I have signed-off my contribution with git commit --amend -s
Lint and unit tests pass locally with my changes (make indent && make check)
I have added tests that prove my fix is effective or that my feature works
I have added the necessary documentation (if appropriate)

egmontkob · 2026-03-13T14:30:57Z

How to test:

Install colors.syntax and update Syntax as per the opening post of #5018.

unset COLORTERM

export TERM=xterm or linux

mcedit -S default catalog.colors (that file is again from #5018)

Should look "obviously broken" without this PR and "obviously fixed" with it.

zyv · 2026-03-13T15:45:02Z

lib/tty/color-ncurses.c

+    // ncurses color pair 0 always refers to the default colors; add it to our hash
+    default_color_pair_id = g_strdup ("-1.-1");
+    mc_ncurses_color_pair = g_try_new (int, 1);
+    *mc_ncurses_color_pair = 0;
+    g_hash_table_insert (mc_tty_ncurses_color_pairs, default_color_pair_id, mc_ncurses_color_pair);
+    mc_tty_ncurses_next_color_pair = 1;


This code... (*)

zyv · 2026-03-13T15:46:41Z

lib/tty/color-ncurses.c

+    color_pair_str = g_strdup_printf ("%d.%d", ifg, ibg);
+
+    mc_ncurses_color_pair =
+        (int *) g_hash_table_lookup (mc_tty_ncurses_color_pairs, (gpointer) color_pair_str);
+
+    if (mc_ncurses_color_pair == NULL)
+    {
+        mc_ncurses_color_pair = g_try_new0 (int, 1);
+        *mc_ncurses_color_pair = mc_tty_ncurses_next_color_pair++;


And this code (*) - can't it be somehow extracted in a function like "remember color pair if not there"?

Done, curious if you like the new version.

I do like it much better, thank you 🙏

zyv · 2026-03-13T15:53:14Z

lib/tty/color-ncurses.c

+    (void) y;
+    (void) x;
+    (void) rows;
+    (void) cols;
+    (void) color;


I know you just moved this code, but could you please use MC_UNUSED instead of this hacky ugliness for the stuff we touch? Or maybe even at the header, something like:

#ifdef ENABLE_SHADOWS tty_colorize_area (int y, int x, int rows, int cols, int color) #else tty_colorize_area (MC_UNUSED int, MC_UNUSED int, MC_UNUSED int, MC_UNUSED int, MC_UNUSED int) #endif

(if this works)

Currently mc uses MC_UNUSED ~10 times and (void) variable_name; ~350 times.

Due to the #ifdef branching nature of things, your proposed variant doesn't look cleaner to me than the current code.

If you don't mind, I'd rather keep this untouched – leave it to you to change if you wish to.

(Your other feedback I agree with and will address.)

Well, shall we (or I) perform a mass search and replace then? The other items accumulated over time before I eventually introduced MC_UNUSED to solve the problem properly because not all warnings can be silenced with the void hack. Only our approach with Andrew so far was to avoid code-base-wide changes after non-essential changes in policy...

Can't we just leave (void) here?

It works here. Whether at other places a similar hack works or not is irrelevant.

By the way, the refactoring I just did (as per your other request) happened to introduce another such (void) trick where MC_UNUSED would be incorrect: the value may or may not be used. What to do then?

You have a very hard time convincing me that I have to switch to the new way which is used at 10 places, the old way used at 350 places isn't good anymore.

I'd also like to firmly reject on principle the idea that if I move a method to a different file (where it obviously should've belonged to from the beginning) then I'd definitely need to update the body of the method to our new standards. It's a fair request that newly written code should use the new way, you may or may not mass-update the old syntax, but moving a method to a different location shouldn't count as new code and shouldn't be subjected to the new guidelines.

In this very case, I'm also reluctant to go with this change because it introduces more duplication that the current one, it's easier to get it wrong in one of the cases (which I guess our CI doesn't test, and I wouldn't want to manually test the no-shadow case either beacuse that's not what this change is about); it would also need to go consistently in the header vs. impl file which adds another ugly ifdef-branching; overall I just find it uglier, more fragile than the current code, and would need additional testing. I'd much rather spend my time on things that actually do matter – there's a lot to do.

(Note: Moving the method is a prerequisite for the main commit of the PR; see the "ouch" comment and the compiler warning in the first two drafts attached to the bug.)

By the way, the refactoring I just did (as per your other request) happened to introduce another such (void) trick where MC_UNUSED would be incorrect: the value may or may not be used. What to do then?

In C++, they have recently adopted maybe_unused 😢, but I think that in C, there is no solution other than accepting the silencer if you cannot change other code (like moving it inside of the disappearing construct).

In my hierarchy of values, I would then accept the silencer as it is less evil than non-generic (duplicating) logic code.

You have a very hard time convincing me that I have to switch to the new way which is used at 10 places, the old way used at 350 places isn't good anymore.

It's unfortunate that I should have a very hard time doing so, because you seem to be otherwise thinking rationally and have quite an experience with C.

The problem with unused variable silencers is that they... well, silence the warnings. However, they are not dedicated unambiguous syntax constructs to do so, but at best, some hopefully otherwise harmless construct, which is accepted to be abused for the purpose.

As such, these statements then cannot be easily identified by means other than semantic analysis and don’t directly convey the meaning to the reader.

A proper way to silence warnings if needed is to use dedicated attributes (which is what the new macro is using). It resolves into a directive to the compiler that doesn't and cannot affect anything else, and can be easily found in the code. Also, its meaning is directly obvious.

Moreover, there are places where the void trick just doesn't work (like labels) and you must use attributes if you need to silence a warning there.

There are several classes of cases when the use of silencers is warranted:

Function parameters

Function returns

Internal variables

The general approach should be to avoid the situation if possible; if not, use it.

The first type of usage I find particularly problematic and want to replace it as a priority

The second is used absolutely inconsistently in our code, and in my opinion should be just thrown away. I don't think that the codebase is ready for strict return checking anyway, and refactoring this properly would be a major project... which might not even make sense in the C world, in terms of what you gain for the amount you invest, unless you're working in some kind of regulated environment. In which case, you might as well reconsider your choice of language.

I can see that in the last case, the macro in its current form can only mark the variable at the declaration site, which doesn't make it much better than the silencer.

Maybe we can improve on that by giving it an optional parameter, so that it would expand only if the parameter evaluates to true. I'm not sure that this is doable within the C macro system and with an amount of effort invested that is worth it.

I'd also like to firmly reject on principle the idea that if I move a method to a different file (where it obviously should've belonged to from the beginning) then I'd definitely need to update the body of the method to our new standards. It's a fair request that newly written code should use the new way, you may or may not mass-update the old syntax, but moving a method to a different location shouldn't count as new code and shouldn't be subjected to the new guidelines.

I don't think that I can be convinced that there is a problem on principle with updating the code as it is touched. In my experience, this is actually the only pragmatic and practical way of evolving huge old code bases in the right direction.

The sweeping changes mostly require an inordinate amount of work and just never get done, and on top of that introduce risks, like mass-upgrades to new types (size_t and friends) or removal of dead casts. You've recently witnessed the effects of that sort yourself.

The approach of doing something to the code that is touched when it is touched, even if it has nothing to do with the change per se (obviously in a separate commit), makes the upgrades possible in the first place and then limits the risks, as the change is more likely to be correct due to its limited nature and more stringent review.

Nevertheless, what is certainly true is that it's highly problematic if we don't agree on the nature of the change and if you have some issues "on principle" with the approach itself. I expected neither of those as I made my suggestion, otherwise I wouldn't have made it.

Now, of course, I'm not going to block anything on these grounds. Let's leave it the way you want for this PR.

The approach of doing something to the code that is touched when it is touched, even if it has nothing to do with the change per se (obviously in a separate commit)

As long as it's a trivial improvement, why not.

In this case it's a combination of two major things:

I do not really consider moving an entire method to a different file – but otherwise leaving it unchanged – as "touching it".

The requested change hardly has any precedence in the source, IMO is absolutely not obvious that it makes the code cleaner (maybe it makes it semantically better, but visually uglier), and if I wanted to be really sure that I didn't make a silly mistake then it would take a bit of additional testing as well.

I hope you don't mind that I just decided to draw the line before this step now. :)

egmontkob · 2026-03-13T17:36:21Z

Note:

In the test file, the last row's almost last columns "base lightgray" and "base white" appear incorrectly. I'll investigate why.

It might be related to the recent removal of the "base" keyword; it might easily be that the test is outdated and the code is fine.

egmontkob · 2026-03-13T23:38:16Z

In the test file, the last row's almost last columns "base lightgray" and "base white" appear incorrectly.

Fixed.

@horkykuba Could you please review the commit "Fix shady trick..."?

-1 for "the terminal's defaults" got converted to -0xff0001 or so, causing troubles further down the line.

Also (I haven't tested this, just looked bad in the code) you explicitly exclude the value 0 from the "shady trick of applying a tint of red", thereby letting ncurses change #000000 to palette index 0. The problem with this is that palette index 0 might be quite far away from a real black. Going with #010000 is much safer.

zyv

I don't think I have anything else to wish for now 👍

zyv · 2026-03-14T06:22:32Z

lib/tty/color-ncurses.c

+    mc_tty_ncurses_next_color_pair = 0;
+    default_color_pair_id = get_ncurses_color_pair (-1, -1);
+    g_assert (default_color_pair_id == 0);
+    (void) default_color_pair_id;  // unused if g_assert is eliminated


Out of curiosity, do you hit the warning when you compile in some configuration, or is this theory?

I'm asking because I've started eliminating these hacks in a separate branch. In the worst case, I'd have to accept that some of those must remain.

Out of curiosity, do you hit the warning when you compile in some configuration, or is this theory?

It's experience (probably mostly from VTE/gnome-terminal).

Look at glib/gtestutils.h: under an #ifdef G_DISABLE_ASSERT these g_assert() and friends are eliminated and don't evaluate (don't even look at) their parameter.

I have also shot myself in the foot with code like g_assert (important_function_call_to_make() == 0); – works great until you disable asserts.

Now, maybe there's no set of configure option to pass to mc which would set this macro, but still, it's bad practice to rely on it.

zyv · 2026-03-14T06:25:09Z

lib/tty/color-ncurses.c

-    init_pair (mc_color_pair->pair_index, ifg, ibg);
-#endif
-    mc_tty_color_save_attr (mc_color_pair->pair_index, attr);
+    ncurses_color_pair = get_ncurses_color_pair (ifg, ibg);


I'd ask to do const int ... here to limit the scope and mutability, and then Andrew will ask exactly the opposite, because he likes to see all variables at the top of the function like in the old, good Pascal days :-(

Maybe you can at least constify it at the top, though?

he likes to see all variables at the top of the function

At the begin of block.

like in the old, good Pascal days :-(

like in the old, good C days .

I'd ask to do const int ... here to limit the scope and mutability

Done – primarily because the line underneath already did the same, so it's now more consistent.

Maybe you can at least constify it at the top, though?

I'm surely misunderstanding you here... declare as const at the top and assign the value later? That's not how const works :-D

zyv · 2026-03-14T07:03:20Z

lib/tty/color-ncurses.c

+    (void) y;
+    (void) x;
+    (void) rows;
+    (void) cols;
+    (void) color;


By the way, the refactoring I just did (as per your other request) happened to introduce another such (void) trick where MC_UNUSED would be incorrect: the value may or may not be used. What to do then?

In C++, they have recently adopted maybe_unused 😢, but I think that in C, there is no solution other than accepting the silencer if you cannot change other code (like moving it inside of the disappearing construct).

In my hierarchy of values, I would then accept the silencer as it is less evil than non-generic (duplicating) logic code.

You have a very hard time convincing me that I have to switch to the new way which is used at 10 places, the old way used at 350 places isn't good anymore.

It's unfortunate that I should have a very hard time doing so, because you seem to be otherwise thinking rationally and have quite an experience with C.

The problem with unused variable silencers is that they... well, silence the warnings. However, they are not dedicated unambiguous syntax constructs to do so, but at best, some hopefully otherwise harmless construct, which is accepted to be abused for the purpose.

As such, these statements then cannot be easily identified by means other than semantic analysis and don’t directly convey the meaning to the reader.

A proper way to silence warnings if needed is to use dedicated attributes (which is what the new macro is using). It resolves into a directive to the compiler that doesn't and cannot affect anything else, and can be easily found in the code. Also, its meaning is directly obvious.

Moreover, there are places where the void trick just doesn't work (like labels) and you must use attributes if you need to silence a warning there.

There are several classes of cases when the use of silencers is warranted:

Function parameters

Function returns

Internal variables

The general approach should be to avoid the situation if possible; if not, use it.

The first type of usage I find particularly problematic and want to replace it as a priority

The second is used absolutely inconsistently in our code, and in my opinion should be just thrown away. I don't think that the codebase is ready for strict return checking anyway, and refactoring this properly would be a major project... which might not even make sense in the C world, in terms of what you gain for the amount you invest, unless you're working in some kind of regulated environment. In which case, you might as well reconsider your choice of language.

I can see that in the last case, the macro in its current form can only mark the variable at the declaration site, which doesn't make it much better than the silencer.

Maybe we can improve on that by giving it an optional parameter, so that it would expand only if the parameter evaluates to true. I'm not sure that this is doable within the C macro system and with an amount of effort invested that is worth it.

I'd also like to firmly reject on principle the idea that if I move a method to a different file (where it obviously should've belonged to from the beginning) then I'd definitely need to update the body of the method to our new standards. It's a fair request that newly written code should use the new way, you may or may not mass-update the old syntax, but moving a method to a different location shouldn't count as new code and shouldn't be subjected to the new guidelines.

I don't think that I can be convinced that there is a problem on principle with updating the code as it is touched. In my experience, this is actually the only pragmatic and practical way of evolving huge old code bases in the right direction.

The sweeping changes mostly require an inordinate amount of work and just never get done, and on top of that introduce risks, like mass-upgrades to new types (size_t and friends) or removal of dead casts. You've recently witnessed the effects of that sort yourself.

The approach of doing something to the code that is touched when it is touched, even if it has nothing to do with the change per se (obviously in a separate commit), makes the upgrades possible in the first place and then limits the risks, as the change is more likely to be correct due to its limited nature and more stringent review.

Nevertheless, what is certainly true is that it's highly problematic if we don't agree on the nature of the change and if you have some issues "on principle" with the approach itself. I expected neither of those as I made my suggestion, otherwise I wouldn't have made it.

Now, of course, I'm not going to block anything on these grounds. Let's leave it the way you want for this PR.

egmontkob · 2026-03-14T10:13:40Z

Changed tty_color_deinit_lib() to reset more variables to their defaults; for consistency and increased robustness.

Will wait for a few days for @horkykuba to possibly chime in, before merging.

Don't modify the value of -1, representing the terminal's default colors. Do add a touch of red to #000000 too since palette index 0 is not necessarily pitch black. Signed-off-by: Egmont Koblinger <egmont@gmail.com>

Remove an unneded tty_setcolor(), fix the array size. Signed-off-by: Egmont Koblinger <egmont@gmail.com>

Signed-off-by: Egmont Koblinger <egmont@gmail.com>

Allocating a new ncurses color pair for every new (fg, bg, attrs) tuple might result in running out of available color pairs too soon. Instead, allocate a new ncurses color pair only if really needed, i.e. if (fg, bg) is a new combination. Apply the attributes separately. Signed-off-by: Egmont Koblinger <egmont@gmail.com>

github-actions bot added needs triage Needs triage by maintainers prio: medium Has the potential to affect progress labels Mar 13, 2026

github-actions bot added this to the Future Releases milestone Mar 13, 2026

egmontkob added area: tty Interaction with the terminal, screen libraries prio: low Minor problem or easily worked around and removed prio: medium Has the potential to affect progress needs triage Needs triage by maintainers labels Mar 13, 2026

egmontkob modified the milestones: Future Releases, 4.9.0 Mar 13, 2026

egmontkob requested review from mc-worker and zyv March 13, 2026 14:27

egmontkob mentioned this pull request Mar 13, 2026

mcedit: some foreground / background color combinations look incorrect #5018

Closed

2 tasks

zyv reviewed Mar 13, 2026

View reviewed changes

egmontkob force-pushed the 5020-ncurses-color-pairs branch from a3519d8 to 18e816e Compare March 13, 2026 17:19

egmontkob force-pushed the 5020-ncurses-color-pairs branch from 18e816e to 64d902f Compare March 13, 2026 23:32

zyv approved these changes Mar 14, 2026

View reviewed changes

mc-worker approved these changes Mar 14, 2026

View reviewed changes

egmontkob force-pushed the 5020-ncurses-color-pairs branch from 64d902f to fb15bdf Compare March 14, 2026 10:08

egmontkob added 4 commits March 17, 2026 11:39

Fix "shady trick" handling ncurses's overlaid colors

5ef1d3d

Don't modify the value of -1, representing the terminal's default colors. Do add a touch of red to #000000 too since palette index 0 is not necessarily pitch black. Signed-off-by: Egmont Koblinger <egmont@gmail.com>

Ncurses shadow drawing cleanup

f3e6ddc

Remove an unneded tty_setcolor(), fix the array size. Signed-off-by: Egmont Koblinger <egmont@gmail.com>

Move tty_colorize_area() from tty-*.c to color-*.c

2a5fbb1

Signed-off-by: Egmont Koblinger <egmont@gmail.com>

egmontkob force-pushed the 5020-ncurses-color-pairs branch from fb15bdf to 95f2560 Compare March 17, 2026 10:48

egmontkob merged commit dc2dbc8 into MidnightCommander:master Mar 17, 2026
10 checks passed

egmontkob deleted the 5020-ncurses-color-pairs branch March 17, 2026 10:55

Uh oh!

Conversation

egmontkob commented Mar 13, 2026

Proposed changes

Checklist

Uh oh!

egmontkob commented Mar 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

egmontkob commented Mar 13, 2026

Uh oh!

egmontkob commented Mar 13, 2026

Uh oh!

zyv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

egmontkob commented Mar 14, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants