Skip to content

Ticket: #5020: Allocate fewer ncurses color pairs #5066

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.

Sign up for GitHub

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

Merged
egmontkob merged 4 commits into from
Mar 17, 2026
Merged

Ticket: #5020: Allocate fewer ncurses color pairs #5066

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5ef1d3d
Fix "shady trick" handling ncurses's overlaid colors
egmontkob Mar 13, 2026
f3e6ddc
Ncurses shadow drawing cleanup
egmontkob Mar 13, 2026
2a5fbb1
Move tty_colorize_area() from tty-*.c to color-*.c
egmontkob Mar 13, 2026
95f2560
Ticket #5020: Reuse ncurses color pairs if possible
egmontkob Mar 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/tty/color-internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ void tty_color_try_alloc_lib_pair (tty_color_lib_pair_t *mc_color_pair);

int tty_maybe_map_color (int color);

void tty_colorize_area (int y, int x, int rows, int cols, int color);

/*** inline functions ****************************************************************************/

#endif
183 changes: 134 additions & 49 deletions lib/tty/color-ncurses.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,57 +49,78 @@

/*** file scope type declarations ****************************************************************/

typedef struct
{
int pair; // ncurses color pair index
int attrs; // attributes
} mc_tty_ncurses_color_pair_and_attrs_t;

/*** forward declarations (file scope functions) *************************************************/

/*** file scope variables ************************************************************************/

static GHashTable *mc_tty_color_color_pair_attrs = NULL;
/*
* Our bookkeeping of the ncurses color pair indices, indexed by the "{fg}.{bg}" string.
*/
static GHashTable *mc_tty_ncurses_color_pairs = NULL;

/*
* Indexed by mc's color index, points to the mc_tty_ncurses_color_pair_and_attrs_t object
* representing the ncurses color pair index and the attributes.
*
* mc's color index represents unique (fg, bg, attrs) tuples. Allocating an ncurses color pair
* for each of them might be too wasteful and might cause us to run out of available color pairs
* too soon (especially in 8-color terminals), if many combinations only differ in attrs.
* So we allocate a new ncurses color pair only if the (fg, bg) tuple is newly seen.
* See #5020 for details.
*/
static GArray *mc_tty_ncurses_color_pair_and_attrs = NULL;

static int mc_tty_ncurses_next_color_pair = 0;

static int overlay_colors = 0;

/* --------------------------------------------------------------------------------------------- */
/*** file scope functions ************************************************************************/
/* --------------------------------------------------------------------------------------------- */

static inline void
mc_tty_color_attr_destroy_cb (gpointer data)
static int
get_ncurses_color_pair (int ifg, int ibg)
{
g_free (data);
}

/* --------------------------------------------------------------------------------------------- */
char *color_pair_str;
int *ncurses_color_pair;
int init_pair_ret;

static void
mc_tty_color_save_attr (int color_pair, int color_attr)
{
int *attr, *key;
color_pair_str = g_strdup_printf ("%d.%d", ifg, ibg);

attr = g_try_new0 (int, 1);
if (attr == NULL)
return;
ncurses_color_pair =
(int *) g_hash_table_lookup (mc_tty_ncurses_color_pairs, (gpointer) color_pair_str);

key = g_try_new (int, 1);
if (key == NULL)
if (ncurses_color_pair == NULL)
{
g_free (attr);
return;
}

*key = color_pair;
*attr = color_attr;

g_hash_table_replace (mc_tty_color_color_pair_attrs, (gpointer) key, (gpointer) attr);
}
ncurses_color_pair = g_try_new0 (int, 1);
*ncurses_color_pair = mc_tty_ncurses_next_color_pair;
#if NCURSES_VERSION_PATCH >= 20170401 && defined(NCURSES_EXT_COLORS) && defined(NCURSES_EXT_FUNCS) \
&& defined(HAVE_NCURSES_WIDECHAR)
init_pair_ret = init_extended_pair (*ncurses_color_pair, ifg, ibg);
#else
init_pair_ret = init_pair (*ncurses_color_pair, ifg, ibg);
#endif

/* --------------------------------------------------------------------------------------------- */
if (init_pair_ret == ERR)
{
g_free (ncurses_color_pair);
g_free (color_pair_str);
return 0;
}

static int
color_get_attr (int color_pair)
{
int *fnd = NULL;
g_hash_table_insert (mc_tty_ncurses_color_pairs, color_pair_str, ncurses_color_pair);
mc_tty_ncurses_next_color_pair++;
}
else
g_free (color_pair_str);

if (mc_tty_color_color_pair_attrs != NULL)
fnd = (int *) g_hash_table_lookup (mc_tty_color_color_pair_attrs, (gpointer) &color_pair);
return (fnd != NULL) ? *fnd : 0;
return *ncurses_color_pair;
}

/* --------------------------------------------------------------------------------------------- */
Expand All @@ -109,6 +130,8 @@ color_get_attr (int color_pair)
void
tty_color_init_lib (gboolean disable, gboolean force)
{
int default_color_pair_id;

(void) force;

if (has_colors () && !disable)
Expand All @@ -122,17 +145,32 @@ tty_color_init_lib (gboolean disable, gboolean force)
tty_use_truecolors (NULL);
}

mc_tty_color_color_pair_attrs = g_hash_table_new_full (
g_int_hash, g_int_equal, mc_tty_color_attr_destroy_cb, mc_tty_color_attr_destroy_cb);
// our tracking of ncurses's color pairs
mc_tty_ncurses_color_pairs = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, g_free);

// ncurses color pair 0 always refers to the default colors; add it to our hash
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
Copy link
Copy Markdown
Member

@zyv zyv Mar 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@egmontkob egmontkob Mar 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// mapping from our index to ncurses's index and attributes
mc_tty_ncurses_color_pair_and_attrs =
g_array_new (FALSE, FALSE, sizeof (mc_tty_ncurses_color_pair_and_attrs_t));
}

/* --------------------------------------------------------------------------------------------- */

void
tty_color_deinit_lib (void)
{
g_hash_table_destroy (mc_tty_color_color_pair_attrs);
mc_tty_color_color_pair_attrs = NULL;
g_hash_table_destroy (mc_tty_ncurses_color_pairs);
mc_tty_ncurses_color_pairs = NULL;

g_array_free (mc_tty_ncurses_color_pair_and_attrs, TRUE);
mc_tty_ncurses_color_pair_and_attrs = NULL;

mc_tty_ncurses_next_color_pair = 0;
}

/* --------------------------------------------------------------------------------------------- */
Expand Down Expand Up @@ -164,36 +202,39 @@ tty_color_try_alloc_lib_pair (tty_color_lib_pair_t *mc_color_pair)

// Shady trick: if we don't have the exact color, because it is overlaid by backwards
// compatibility indexed values, just borrow one degree of red. The user won't notice :)
if ((ifg & FLAG_TRUECOLOR) != 0)
if (ifg >= 0 && (ifg & FLAG_TRUECOLOR) != 0)
{
ifg &= ~FLAG_TRUECOLOR;
if (ifg != 0 && ifg <= overlay_colors)
if (ifg <= overlay_colors)
ifg += (1 << 16);
}

if ((ibg & FLAG_TRUECOLOR) != 0)
if (ibg >= 0 && (ibg & FLAG_TRUECOLOR) != 0)
{
ibg &= ~FLAG_TRUECOLOR;
if (ibg != 0 && ibg <= overlay_colors)
if (ibg <= overlay_colors)
ibg += (1 << 16);
}

#if NCURSES_VERSION_PATCH >= 20170401 && defined(NCURSES_EXT_COLORS) && defined(NCURSES_EXT_FUNCS) \
&& defined(HAVE_NCURSES_WIDECHAR)
init_extended_pair (mc_color_pair->pair_index, ifg, ibg);
#else
init_pair (mc_color_pair->pair_index, ifg, ibg);
#endif
mc_tty_color_save_attr (mc_color_pair->pair_index, attr);
const int ncurses_color_pair = get_ncurses_color_pair (ifg, ibg);
const mc_tty_ncurses_color_pair_and_attrs_t pair_and_attrs = { .pair = ncurses_color_pair,
.attrs = attr };

g_array_insert_val (mc_tty_ncurses_color_pair_and_attrs, mc_color_pair->pair_index,
pair_and_attrs);
}

/* --------------------------------------------------------------------------------------------- */

void
tty_setcolor (int color)
{
mc_tty_ncurses_color_pair_and_attrs_t *pair_and_attrs;

color = tty_maybe_map_color (color);
attr_set (color_get_attr (color), color, NULL);
pair_and_attrs = &g_array_index (mc_tty_ncurses_color_pair_and_attrs,
mc_tty_ncurses_color_pair_and_attrs_t, color);
attr_set (pair_and_attrs->attrs, pair_and_attrs->pair, NULL);
}

/* --------------------------------------------------------------------------------------------- */
Expand Down Expand Up @@ -264,3 +305,47 @@ tty_use_truecolors (GError **error)
}

/* --------------------------------------------------------------------------------------------- */

void
tty_colorize_area (int y, int x, int rows, int cols, int color)
{
#ifdef ENABLE_SHADOWS
cchar_t *ctext;
wchar_t wch[CCHARW_MAX + 1];
attr_t attrs;
short color_pair;

if (!use_colors || !tty_clip (&y, &x, &rows, &cols))
return;

color = tty_maybe_map_color (color);
color = g_array_index (mc_tty_ncurses_color_pair_and_attrs,
mc_tty_ncurses_color_pair_and_attrs_t, color)
.pair;

ctext = g_malloc (sizeof (cchar_t) * (cols + 1));

for (int row = 0; row < rows; row++)
{
mvin_wchnstr (y + row, x, ctext, cols);

for (int col = 0; col < cols; col++)
{
getcchar (&ctext[col], wch, &attrs, &color_pair, NULL);
setcchar (&ctext[col], wch, attrs, color, NULL);
}

mvadd_wchnstr (y + row, x, ctext, cols);
}

g_free (ctext);
#else
(void) y;
(void) x;
(void) rows;
(void) cols;
(void) color;
Comment on lines +343 to +347
Copy link
Copy Markdown
Member

@zyv zyv Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor Author

@egmontkob egmontkob Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Copy Markdown
Member

@zyv zyv Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Copy Markdown
Contributor Author

@egmontkob egmontkob Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Copy Markdown
Member

@zyv zyv Mar 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Function parameters
  2. Function returns
  3. 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.

Copy link
Copy Markdown
Contributor Author

@egmontkob egmontkob Mar 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)

#endif
}

/* --------------------------------------------------------------------------------------------- */
12 changes: 12 additions & 0 deletions lib/tty/color-slang.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,3 +233,15 @@ tty_use_truecolors (GError **error)
}

/* --------------------------------------------------------------------------------------------- */

void
tty_colorize_area (int y, int x, int rows, int cols, int color)
{
if (use_colors)
{
color = tty_maybe_map_color (color);
SLsmg_set_color_in_region (color, y, x, rows, cols);
}
}

/* --------------------------------------------------------------------------------------------- */
2 changes: 0 additions & 2 deletions lib/tty/tty-internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,6 @@ char *mc_tty_normalize_from_utf8 (const char *str);
void tty_init_xterm_support (gboolean is_xterm);
int tty_lowlevel_getch (void);

void tty_colorize_area (int y, int x, int rows, int cols, int color);

/*** inline functions ****************************************************************************/

#endif
Loading
Loading