Color handling refactoring #4995
Conversation
github-actions
bot
added
needs triage
egmontkob
force-pushed
the
color-handling-refactor
branch
2 times, most recently
from
85b624c to
ac6cacb
Compare
zyv
left a comment
zyv
left a comment
There was a problem hiding this comment.
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
left a comment
mc-worker
left a comment
There was a problem hiding this comment.
Cool! Thank you very much!
lib/skin/colors.c
Outdated
| /* | ||
| * Helper for mc_skin_color_cache_init () | ||
| */ | ||
| static void |
lib/tty/tty-internal.c
Outdated
| /* --------------------------------------------------------------------------------------------- */ | ||
|
|
||
| int | ||
| tty_maybe_map_color (int color) |
There was a problem hiding this comment.
Let's move it to color-internal.[ch].
There was a problem hiding this comment.
Obviously, that's a much better place. I didn't realize we had such a file :) Done.
lib/tty/tty-internal.c
Outdated
| { | ||
| if (color >= COLOR_MAP_OFFSET) | ||
| return tty_color_role_to_pair[color - COLOR_MAP_OFFSET]; | ||
| else |
There was a problem hiding this comment.
else after return is unnecessary.
…dget Signed-off-by: Egmont Koblinger <egmont@gmail.com>
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
force-pushed
the
color-handling-refactor
branch
from
ac6cacb to
742de5e
Compare
| */ | ||
| #define COLOR_MAP_OFFSET 4096 | ||
|
|
||
| enum |
There was a problem hiding this comment.
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.
zyv
added
area: skin
3 participants
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_COLORetc. 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 > 0checks rather thanret >= 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& 0x7Fmask 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_colorsetc. 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_COLORetc. from the upper level skin layer to the lower level tty layer becausetty_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.