Ticket #4995: color handling refactoring by egmontkob · Pull Request #4995 · MidnightCommander/mc

egmontkob · 2026-02-05T16:53:26Z

What would you guys think of a color handling refactoring along these lines?

The current code does the resolution from the role (e.g. "dialog's title color") to the ncurses/slang color pair id as soon as possible.

A significant drawback of this design is that initializing some color constant (e.g. the ones used for normal, error, listbox and help dialogs), or later adjusting these values upon a runtime skin change are pretty cumbersome. Either some locally used colors need to be defined in a much more global context (pointed out as unfortunate design twice during code review: #4916 (comment) and #4976 (comment)), or the code that switches the skin needs to dig deep down into the territory of individual widgets to change the color they use.

My idea is to instead carry the color role identifier for as long as possible, and only resolve to the actual color when it comes to painting. This way only the color cache needs to be updated from the new skin; widget's idea on which color (i.e. color role) to use remains the same.

We have those 80-ish uppercase variables like CORE_NORMAL_COLOR etc. Currently it's pretty counterintuitive that despite the uppercase naming, these are variables and do change at a skin change. Yet, for some reason, they aren't declared as individual variables but as members of an array, so we have to maintain their indexing and manually renumber every time a new item is added.

With my suggestion, these become constants (actually enum values) as their name suggests, and we no longer have to manually index them.

Now, due to having two places in mc that bypass these variables, namely file highligthing and editor syntax highlighting, the story becomes a bit more complex. Sometimes we know the color role id, yet to be resolved to an ncurses/slang color pair id, but sometimes the latter one is the only thing we have.

Might not be the cleanest design per se, but I just distinguish these two based on how large the value is: if the value is low then it refers directly to the ncurses/slang color pair id; if the value is large (above the arbitrarily picked 4096) then it's a color role id which still needs a hop to resolve. This sort of pattern already exists in mc's file highlighting code where negative vs. positive values have different roles, although it looks to me it's legacy leftover because both are just color pair id, the negative ones will simply be flipped to positive (and, by the way, due to two faulty ret > 0 checks rather than ret >= 0, color pair id 0 (i.e. the terminal's default colors) can't be used for file highlighting; something that probably nobody cares about). I've ported this negative vs. positive split to the new ranges.

No more conceptual tty_setcolor() vs. tty_lowlevel_setcolor() separation (although it was probably a legacy anyway, they were identical, except for a & 0x7F mask in slang that I don't understand), now by design there's a single method only which decides based on the value (small vs. large) how to handle it.

As you can see, the initialization of command_colors etc. becomes compile-time rather than runtime, no need to reinitialize at skin change, and easy to move all of them where they really belong.

I've moved CORE_NORMAL_COLOR etc. from the upper level skin layer to the lower level tty layer because tty_setcolor() now needs to know about them, and I didn't want tty to know about skin. This is debatable where it should really go, what the new variables / types / etc. should be named, and so on, suggestions welcome.

Let me know please how you like the overall direction.

zyv

I've had a look at it, and mechanically your rework definitively seems like a much better way to achieve the same goal. My understanding of the whole system, however, is so limited that I can't judge if this approach introduces some new problems or not, and if there is an even better way to do it. I'm sorry, but I would like to defer to Andrew on this...

mc-worker

Cool! Thank you very much!

mc-worker · 2026-02-06T16:36:13Z

lib/skin/colors.c

+/*
+ * Helper for mc_skin_color_cache_init ()
+ */
+static void


Let's make it inline.

mc-worker · 2026-02-06T16:37:37Z

lib/tty/tty-internal.c

 /* --------------------------------------------------------------------------------------------- */
+
+int
+tty_maybe_map_color (int color)


Let's move it to color-internal.[ch].

Obviously, that's a much better place. I didn't realize we had such a file :) Done.

mc-worker · 2026-02-06T16:41:06Z

lib/tty/tty-internal.c

+{
+    if (color >= COLOR_MAP_OFFSET)
+        return tty_color_role_to_pair[color - COLOR_MAP_OFFSET];
+    else


else after return is unnecessary.

mc-worker · 2026-02-08T09:48:15Z

lib/tty/color.h

+ */
+#define COLOR_MAP_OFFSET 4096
+
+enum


I think the lib/tty layer should not know which basic widgets and "applications" (editor, viewer, ...) are implemented. Let's move color definitions and related macros back to lib/skin.

Sorry for the long delay. I was thinking of a truly clean solution, but I didn't find one.

I did the change you requested, but now we need to have two lib/tty/* -> #include "lib/skin.h" dependencies. Not sure if it's better. What do you think?

"there is no problem in cs that cannot be solved with another layer of indirection" ... 🤣

but more seriously, if you reframe a skin as the color palette, then it becomes obvious that it is intrinsically linked with color resolution. one could avoid that only by making the system way more dynamic, but that would also make it much heavier. so i'm not convinced that andrew's assertion holds up, but i haven't looked into it very deeply.

I did the change you requested, but now we need to have two lib/tty/* -> #include "lib/skin.h" dependencies. Not sure if it's better. What do you think?

I think I've fixed this: edf652e. What do you think?

I think I've fixed this: edf652e. What do you think?

I generally like your approach, thanks!

Two nitpicks:

You've added a TTY_ prefix to COLOR_MAP_OFFSET, don't you want to carry it consistently to COLOR_MAP_NEXT and COLOR_MAP_SIZE, too?

With the new g_array business, it's essential that we've called tty_init_colors() but not tty_colors_done(). Otherwise a color lookup accesses random uncontrolled parts of the memory. What do you think of setting tty_color_role_to_pair to NULL at its definition/initialization as well as in tty_colors_done()? It'd make broken code more obviously broken, accessing a NULL pointer rather than some random address.

And one more question:

I'm not familiar with this "fixup" workflow... is it that you simply manually begin the commiit message with fixup!, and then hitting the big green Merge button in GitHub will automatically squash it? Or what's the exact workflow with them?

Thanks!

I think I've fixed this

Probably, usage of GArray is overdesign. Simple int * is enough.

Probably, usage of GArray is overdesign. Simple int * is enough.

Indeed, good ol' malloc/free is just fine.

I'm not familiar with this "fixup" workflow... is it that you simply manually begin the commiit message with fixup!, and then hitting the big green Merge button in GitHub will automatically squash it? Or what's the exact workflow with them?

You can edit things and then git add -p and git commit --fixup REF to mark them as belonging to a particular commit. After that, if you do git rebase -i --autosquash, it will automatically reorder marked commits and fuse them together. This is what my rebase bot does among other things. It's useful for self-review when you check your own patch series and make many small adjustments. Or to show you what I want to change more exactly without having you do a range diff on an already rebased patches. Check the manual for more details.

You've added a TTY_ prefix to COLOR_MAP_OFFSET, don't you want to carry it consistently to COLOR_MAP_NEXT and COLOR_MAP_SIZE, too?

No. COLOR_MAP_OFFSET is defined in lib/tty, others aren't.

With the new g_array business...

Done.

I'm not familiar with this "fixup" workflow... is it that you simply manually begin the commiit message with fixup!

Such message is generated by git commit --fixup COMMIT_TO_FIX.

and then hitting the big green Merge button in GitHub will automatically squash it?

Can't say. I do everything on git-client side. All "fixup!" commits are squashed with target ones using git rebase -i --autosquash ....

and then hitting the big green Merge button in GitHub will automatically squash it?

Can't say. I do everything on git-client side. All "fixup!" commits are squashed with target ones using git rebase -i --autosquash ....

I have added merge checks that should ideally prevent exactly that. You might be able to bypass it as an administrator, but generally the idea is to make sure fixups don't get merged. To get rid of them, one would use the rebase bot or do it manually like Andrew.

egmontkob · 2026-02-20T08:18:14Z

(just a rebase; the review comment is yet to be addressed)

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

ossilator · 2026-02-21T11:03:39Z

lib/skin/colors.c

-    DIFFVIEWER_ERROR_COLOR = mc_skin_color_get ("diffviewer", "error");
-
-    FILEHIGHLIGHT_DEFAULT_COLOR = mc_skin_color_get ("filehighlight", "_default_");
+    mc_skin_one_color_init (CORE_DEFAULT_COLOR, "skin", "terminal_default_color");


given how uniform this code is, i would recommend a data-driven approach here.

If I understand correctly what you mean here then I agree, done.

ossilator · 2026-02-21T11:08:55Z

lib/skin.h

-#define FILEHIGHLIGHT_DEFAULT_COLOR  mc_skin_color__cache[78]
-
-#define MC_SKIN_COLOR_CACHE_COUNT    79
+#define COLOR_MAP_OFFSET 4096


you could just put that inside the/an enum as well.

Could you please be more specific? I can easily do this by "wasting" the first number in the enum, and correspondingly in the array of ints, but I'd prefer not to. Or the code would be fragile because elsewhere we'd need to know the symbolic name of the first entry and it'd break if someone changes the order.

enum { COLOR_MAP_OFFSET = 4096, /* Basic colors */ CORE_DEFAULT_COLOR = COLOR_MAP_OFFSET, ...

I didn't know enums could have repeated values :) but Andrew's change is incompatible with your idea, so I guess we'll just leave his version.

ossilator · 2026-02-21T11:21:13Z

lib/tty/color.h

+ */
+#define COLOR_MAP_OFFSET 4096
+
+enum


"there is no problem in cs that cannot be solved with another layer of indirection" ... 🤣

but more seriously, if you reframe a skin as the color palette, then it becomes obvious that it is intrinsically linked with color resolution. one could avoid that only by making the system way more dynamic, but that would also make it much heavier. so i'm not convinced that andrew's assertion holds up, but i haven't looked into it very deeply.

egmontkob · 2026-02-21T15:08:00Z

but more seriously, if you reframe a skin as the color palette

Can't figure out how to reply to a comment on a file which is now outdated. (Yes, I know, we should switch to gerrit :))

Your comment makes sense, it might make sense to reposition this part of the source code as "palette" rather than "skin", but at the same time it is also a quite vague direction and I can't see either how the exact details should go, and for that reason preferably I wouldn't spend time on this right now.

ossilator · 2026-02-22T10:14:00Z

Can't figure out how to reply to a comment on a file which is now outdated.

following the link in the notification mail takes you directly to it, where it's possible to reply as usual. but yeah, gerrit is so much better at this ...

zyv · 2026-02-22T10:48:53Z

but more seriously, if you reframe a skin as the color palette

Can't figure out how to reply to a comment on a file which is now outdated. (Yes, I know, we should switch to gerrit :))

Click on the "Files changed" tab and then on the grey "Comments" button on the right next to the green "Submit review".

egmontkob · 2026-02-22T16:53:46Z

Thx guys!

Just pushed the autosquashed version.

I think it's fine to merge; right?

Once merged, let's move on to #4976 which was waiting for this one.

zyv

Fine with me, with one small comment...

zyv · 2026-02-22T18:36:06Z

lib/skin/colors.c

-    DIFFVIEWER_ERROR_COLOR = mc_skin_color_get ("diffviewer", "error");
-
-    FILEHIGHLIGHT_DEFAULT_COLOR = mc_skin_color_get ("filehighlight", "_default_");
+    for (unsigned int i = 0; i < G_N_ELEMENTS (color_keywords); i++)


Can unsigned int here be replaced with size_t? I'd stay away from old-style types if possible at all; they always only bring uncertainty, confusion, and portability problems.

Can unsigned int here be replaced with size_t?

Done.

"int" is optimized for the machine word size, and unless there is a good reason to actually consider values >4G (mc is 32+ bit only) or require a fixed word size for protocol reasons, using anything else than (unsigned) int is kinda counter-productive.

Variables referring to a color pair may now contain either the lowlevel color pair id, or an identifier of the color role, the latter to be looked up in a map according to the skin. By using the color role id for as long as we can, rather than the resolved color pair id, initializing colors and later changing skins becomes much more straightforward. Signed-off-by: Egmont Koblinger <egmont@gmail.com>

egmontkob requested review from mc-worker and zyv February 5, 2026 16:53

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

github-actions bot added this to the Future Releases milestone Feb 5, 2026

egmontkob force-pushed the color-handling-refactor branch 2 times, most recently from 85b624c to ac6cacb Compare February 5, 2026 17:19

zyv reviewed Feb 5, 2026

View reviewed changes

mc-worker reviewed Feb 6, 2026

View reviewed changes

mc-worker mentioned this pull request Feb 7, 2026

Add MashDark skin #5006

Open

5 tasks

egmontkob force-pushed the color-handling-refactor branch from ac6cacb to 742de5e Compare February 7, 2026 17:45

egmontkob mentioned this pull request Feb 8, 2026

Ticket #4974: Skinnable hint bar, prompt and cmdline; plus other cleanups #4976

Draft

5 tasks

mc-worker reviewed Feb 8, 2026

View reviewed changes

zyv added area: skin Theming support and skin files and removed needs triage Needs triage by maintainers labels Feb 9, 2026

This was referenced Feb 19, 2026

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

Closed

Ticket #5018: Color pairs beyond 256 #5019

Merged

egmontkob force-pushed the color-handling-refactor branch from 742de5e to 617cfc7 Compare February 20, 2026 08:17

Rename the input widget's color enums to be consistent with dialog wi…

8a20ed8

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

egmontkob force-pushed the color-handling-refactor branch from 617cfc7 to 4677b61 Compare February 21, 2026 09:51

ossilator reviewed Feb 21, 2026

View reviewed changes

egmontkob force-pushed the color-handling-refactor branch from 4677b61 to d9203ed Compare February 21, 2026 14:56

egmontkob force-pushed the color-handling-refactor branch from 45659bc to a76a2e3 Compare February 22, 2026 16:52

zyv changed the title ~~Color handling refactoring~~ Ticket #4995: color handling refactoring Feb 22, 2026

zyv approved these changes Feb 22, 2026

View reviewed changes

egmontkob force-pushed the color-handling-refactor branch from a76a2e3 to a101d98 Compare February 22, 2026 19:25

Uh oh!

Comments

Conversation

egmontkob commented Feb 5, 2026

Uh oh!

zyv left a comment

Choose a reason for hiding this comment

Uh oh!

mc-worker 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!

Choose a reason for hiding this comment

Uh oh!

egmontkob Feb 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Feb 20, 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!

egmontkob commented Feb 21, 2026

Uh oh!

ossilator commented Feb 22, 2026

Uh oh!

zyv commented Feb 22, 2026

Uh oh!

egmontkob commented Feb 22, 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!

Reviewers

Assignees

egmontkob Feb 21, 2026 •

edited

Loading