Ticket #2169 (closed defect: fixed)

Opened 6 years ago

Last modified 6 years ago

[Patch] I can has 256 colorz

Reported by: egmont Owned by: slavazanko
Priority: critical Milestone: 4.8.0-pre1
Component: mc-tty Version: master
Keywords: 256colors Cc: zaytsev
Blocked By: Blocking:
Branch state: Votes for changeset: commited-master

Description

Tadaaa! The feature you've all been waiting for!

Forget the old limit of 8 background and 16 foreground colors. From now on Midnight Commander can use all the 256 colors, as your favorite terminal emulator supports them - or not, in which case it's not going to stay your favorite terminal for long.

And... just here, just now: Order one right now (dial 1-800-256-COLORZ), and get a free bonus: the ability to specify some non-color attributes, including bold (for any color, no longer confused with brighness), underline (ideal for hotkeys), and blink (strictly only for those under the age of 10).

And if one bonus wasn't convincing enough, you get a second extra, too! An example skin that's totally different from the current ones (dark on bright, making it a "noon commander"), heavily using 256 colors and the new attributes. Don't worry, it does not blink!

If you're a user, just apply the patch, compile and install mc, open a decent terminal emulator (e.g. xterm, gnome-terminal, konsole, iterm, putty, and probably lots of others) and start mc with this command:

TERM=xterm-256color mc --skin=sand256

Should you want to create your own brand new skin, everything's described in the updated documentation and in the sand256.ini file.

If you're an mc developer, please read on here for some boring technical details behind the design decisions I made in my patch. Otherwise you can stop here and enjoy the patch :)

-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-

Using the legacy colors, there's a huge confusion whether the single "bold" attribute actually means "bold" (so only 8 colors in total) or "bright" (ending up with 16 foreground and 8 background colors) or both bold and bright at the same time. The actual behavior depends on the terminal itself. 256 colors are supposed to end this confusion. The first 16 of these colors are supposed to correspond to the legacy 8 normal and 8 bright colors, and bold becomes a separate flag applicable to any of them. E.g. you can have red in normal width, red in bold, brightred in normal width, and brightred in bold.

If your $TERM is set up for 256 colors, current mc behaves differently for bright colors depending on whether it uses slang or ncurses. If compiled with ncurses, mc itself translates the word "brightred" into normal red (color 1) with the bold attribute. If using slang, slang interprets the word "brightred", and it uses the correct semantics when 256 colors are available: it really becomes brightred (color 9), without being bold. My patch modifies the ncurses version behave the same way as the slang version does: interpret the word "brightred" correctly when 256 colors are available. (And of course the same for all 8 colors - red is just an example.) Apart from this change, my patch shouldn't introduce any other visible changes to any currently valid color configurations - let me know if it's not the case.

Currently a cell's look is specified by two fields: foreground and background color. The "bold/bright" flag, with is weird semantics, is incorporated in the foreground color both at the user-facing places (e.g. you have to specify "brightred" instead of "red" as foreground color in the config file), as well as in the source (the actual foreground value becomes COLOR_RED | A_BOLD).

The way 256 colors clearly separate the color itself from the boldness, as well as the desire to add support for underlined characters required me to revise this situation. Combining the attributes with the foreground color would have caused a lot of confusion and hacks both in the config file format as well as the internal representation. So I decided to separate them, as they are actually separate properties. From now on there are three fields: foreground, background and attributes. (When in 8 color mode, the brightness of the fg color and the bold attribute will still eventually be merged into a single property, e.g. "brightred;black" and "red;black;bold" will end up being the same color - but not when in 256 color mode.) Slang does the magic automatically on its own (interprets "brightred" differently for 8-color and 256-color terminals), for ncurses we manually do this right before allocating the colors with init_pair(), that is, keep the two properties separate as long as we can.

The structure containing these fg + bg + attr values is oddly still called color_pair, maybe it could be renamed. Not very surprisingly, separating the three properties often ended up in a cleaner code with less black magic.

So instead of two fields only, now a third field can be added containing the attributes. This works everywhere (I believe), I've tested in the command line, in the MC_COLOR_TABLE env var, in skin files, and in editor's syntax highlight files. Multiple attributes should be appended by the "+" sign, I chose this sign because it's intuitive (not only for geeks who'd probably prefer the bitwise OR "|"), and it's not a special shell character, so you can still specify any colors and attributes in the command line without worrying about quoting or escaping them. Since the empty string has the special meaning of falling back to the default, and the default might have some attributes, there has to be a way to specify "no flags": you can use any nonempty invalid string (such as "none").

The editor syntax highlight rules required to be able to specify the third field (attributes) without specifying the second (that is, not to modify the background). Unfortunately the whitespace-separated config file format does not allow it. Therefore I chose to introduce the special word "base" meaning mc's base colors.

A note on cooledit compatibility: cooledit exits with "Bus error" seeing an extra token (attributes) at the end of the line. Given that it's clearly not by design but due to a bug, given that's cooledit has one of the ugliest UIs I've ever seen, given that despite being an X application it supports a stunning number of 27 (yep: twenty-seven) colors (instead of 16 million) for no particular reason, and given that cooledit's not been maintained in the last 5 years, I just don't think it's important at all.

About color names: The words "color0" to "color255" are directly understood by slang, just as legacy color names ("red" and such). 256colors.pl (distributed in xterm's source) shows the colors using their 0..255 numbers, so I think it's convenient to allow these numbers to be used in the config files. On the other hand, I believe that names such as "rgb123" and "gray7" (inspired by the joe text editor, see its c.jsf file) are more intuitive and clearly more readable, similarly to the English names of legacy colors.

These are not understood by slang though, so mc has to parse and convert them. For the legacy 16 colors I decided to use the English names as the canonical ones rather than color0..color15, that is, the English names are sent to slang (though it equally understands color0..color15 too). This is for better compatibility: I haven't checked whether all ancient slangs support the colorN names, but this way we don't have to worry about it.

Let me know if you have any questions or concerns, I'm happy to answer them.

Attachments

mc-I_can_has_256_colorz_skrinshut.png (99.0 KB) - added by egmont 6 years ago.
Skrinshut
mc-4.7.0.5-I_can_has_256_colorz.patch (35.4 KB) - added by egmont 6 years ago.
Ze patch for 4.7.0.5
mc-4.7.2-I_can_has_256_colorz.patch (37.1 KB) - added by egmont 6 years ago.
Ze patch for 4.7.2

Change History

Changed 6 years ago by egmont

Skrinshut

Changed 6 years ago by egmont

Ze patch for 4.7.0.5

Changed 6 years ago by egmont

Ze patch for 4.7.2

comment:1 Changed 6 years ago by angel_il

OMG, it works! :) cool!

comment:2 Changed 6 years ago by zaytsev

Egmont, holy shit! This is the most entertaining bug report I have ever read... and it actually DOES work. Where's the catch?! You must be insane...

comment:3 Changed 6 years ago by zaytsev

  • Cc zaytsev added

comment:4 Changed 6 years ago by egmont

Catch?? LOL. There's no catch. Really :)

I used to be a totally sane person. Until the moment I started working on this feature.

Understanding how ncurses and slang use 256 colors... Seeing that for the 0..15 colors of the 256 slang used the old-fashioned escapes on Mac but the new ones on Linux, while ncurses used the new ones on both... why? Difference in terminfo, in slang, or where? Bug? Or feature? Don't know... Seeing that gray23 didn't work in slang (reported upstream)... Not being able to compile the ncurses version on my mac, so sshing back and forth between two computers, including terminal windows over ssh, getting totally confused all the time: which host does this window belong to? Am I starting the slang or the ncurses build? The stable or unstable version of mc? Which version of my patch? Which skin file?... Testing with all kinds of terminal emulators... and seeing how totally differently they all behave based on their settings (e.g. disabling bold) and seeing the difference in the interpretation of the old and new escape sequences for the first 16 colors (e.g. you choose red (color 1) with the 256-color escape, and then make it bold, should it become brighter too? I believe no, but in some terminals it does.) Reporting the bug to vte (gnome-terminal), eventually creating a patch too... Maintaining the mc patch for two branches in parallel... and a mistake made in merge revealing another vte bug, also reported and fixed... All those hours spending creating a reasonably looking skin... Testing for a couple of days, then waiting for the new versions to come out, with the misbelief that 4.7.2 was going to be a stable release and I could drop the patch for 4.7.0.x... Writing most the bugreports in advance in a text editor... Watching the release happening, porting the patches as fast as I could... And some final testing again... comparing the two patches to see if they look kinda same, I didn't forget something from one of them... And, of course, sometimes accidentally deleting what I did in the last couple of minutes...

Yep, all these things did make me go insane :D

comment:5 Changed 6 years ago by slavazanko

  • Owner set to slavazanko
  • Status changed from new to accepted

egmont, really cool :). Must be in next releases...

comment:6 Changed 6 years ago by slavazanko

created branch 2169_256colors

Changesets:

Review, please,

Egmont, this feature not for stable. But for master (devel branch) it's very and very usable :)
Thanks!

comment:7 Changed 6 years ago by slavazanko

  • severity changed from no branch to on review

comment:8 Changed 6 years ago by andrew_b

  • Version changed from version not selected to master

egmont, thanks a lot for a great work! Works fine!

I added a Russian translation of 256 colors support in MC main man page.

comment:9 Changed 6 years ago by egmont

Please look at #2170 and #2171 too, the example skin would only be complete if those two issues are addressed as well. In #2171 I have a question for both of the patches - I don't understand what "focused" means and I'm not sure if you're fine with reverting the colors of the F2 and Alt-e menus. I'm waiting for your help there to complete those patches.

Minor issue with the 256 color patch here: I just realized I forgot to handle if malloc returns NULL. Could you please fix that for me?

comment:10 Changed 6 years ago by angel_il

  • Milestone changed from 4.7.3 to 4.7.4

comment:11 Changed 6 years ago by zaytsev

  • Milestone changed from 4.7.4 to 4.7.5

comment:12 Changed 6 years ago by slavazanko

  • Votes for changeset set to slavazanko

comment:13 Changed 6 years ago by angel_il

  • Votes for changeset changed from slavazanko to slavazanko angel_il
  • severity changed from on review to approved

comment:14 Changed 6 years ago by slavazanko

  • Status changed from accepted to testing
  • Votes for changeset changed from slavazanko angel_il to commited-master
  • Resolution set to fixed
  • severity changed from approved to merged
  • Milestone changed from 4.7.5 to 4.8.0-pre1

comment:15 Changed 6 years ago by slavazanko

  • Status changed from testing to closed

comment:16 Changed 6 years ago by slavazanko

  • Status changed from closed to reopened
  • Votes for changeset commited-master deleted
  • severity changed from merged to on rework
  • Priority changed from major to critical
  • Keywords 256colors added
  • Resolution fixed deleted

So, have troubles with back compatibility. For example, compile && run mc with dark skin on standart terminal (non-256 colours):

mc -S dark

You'll see blinked menubar.

Created branch changeset:2169_fix_colors, but branch now incomplete: need to fix --with-ncurses compilation case (in this case menubar is invisible).

Egmont, can you help with this?

comment:17 Changed 6 years ago by egmont

I'm happy to help, but could you please give me clear instructions on how to get to the exact same source code that you have? I do not know anything about git and not planning to learn it right now. I'm happy to download tarballs, apply patches, or execute exact commands that you tell me :)

comment:18 Changed 6 years ago by slavazanko

  • severity changed from on rework to on review

Generally I have fixed bug in ncurses. :) Thank you for reply.

So, for developers: branch 2169_fix_colors, changesets:

Review, please.

comment:19 Changed 6 years ago by angel_il

egmont:
how to work with git
mkdir devel
git clone git://midnight-commander.org/git/mc.git
cd mc
git checkout -b 2169_fix_colors origin/2169_fix_colors
..do some..
git add changed_file.1 changed_file.2 changed_file.3
git commit
git format-patch origin/master for make patch files from faster to last commit

attache your file(s)...

how to see trouble
git checkout master
autogen.sh
configure
make
start "src/mc -S dark"

comment:20 Changed 6 years ago by angel_il

*faster* = master

comment:21 Changed 6 years ago by andrew_b

  • Votes for changeset set to andrew_b
  • Type changed from enhancement to defect

comment:22 Changed 6 years ago by egmont

Hi slavazanko,

I could reproduce the problem, and verified that your patches fix it for both slang and ncurses.

(Note that the current version of your first patch in the branch, unlike the version linked above, has the "attr |= (A_BOLD | A_REVERSE);" line commented out, so you do not set any attributes if the background color is above 8, only reset that bit to move it to the 0-7 range.)

Please let me know if there's anything left that I can do for you.

Thanks a lot!

comment:23 Changed 6 years ago by slavazanko

  • Status changed from reopened to accepted

Yep, code commented now. Reason is: not all terminal have same behaviour while processed these attributes. For example, some tables with different terminals, attributes and reaction:

Konsole (KDE):

A_BLINKblinked text, no highlight for background
A_BOLD | A_REVERSEhighlight for background

xterm (261-2.fc14.x86_64), gnome-terminal:

A_BLINKblinked text, no highlight for background
A_BOLD | A_REVERSEreversed foreground and background colors, highlight for background

rxvt:

A_BLINKhighlight for background
A_BOLD | A_REVERSEreversed foreground and background colors, highlight for background

I asked Thomas Dikey (Ncurses maintainer) about background highlighting. In addition, I provided to Thomas some info about SLtt_Blink_Mode - this is a S-Lang option for toggle A_BlINK attribute between background highlight and text blinking... Some piece of answer:

Just reading slang's code, I don't see any special handling. There are some comments:

   /* Although xterm cannot blink, it does display the blinking characters
    * as bold ones.  Some Rxvt will display the background as high intensity.
    */
   if ((NULL == (Blink_Vid_Str = tt_tgetstr("mb")))
       && is_xterm)
     Blink_Vid_Str = "\033[5m";

and

        if (fgbg & SLTT_BLINK_MASK)
          {
             /* Someday Linux will have a blink mode that set high intensity
              * background.  Lets be prepared.
              */
             if (SLtt_Blink_Mode) tt_write_string (Blink_Vid_Str);
          }

It looks as if it simply assumes that the blink attribute is used only to set a bright background.  (Not so long ago, slang used to assume that vt100's did color, etc., though as a result of my comments, most of that hardcoded behavior went away).

So... for devices where this happens to work (and there's no reliable
way to determine this automatically), just setting A_BLINK would do what
you're asking. 

So, my resolution: no background highlight for compilation with ncurses. In all cases colors should be displayed the same. This possible with no attributes for background (and yes - only eight color for background).

If someone have ideas about fixing this trouble - welcome, we strongly need help in this situation.

comment:24 Changed 6 years ago by slavazanko

  • Votes for changeset andrew_b deleted

Need to review again. New commit in branch:

review again, please

comment:25 Changed 6 years ago by egmont

Fyi: Thanks for doing all these things... However, I think that a skin telling about itself whether it uses 256 colors is pretty fragile. It should ideally be autodetected when parsing the skin file. I don't know how hard it is to implement that, probably not that hard. Anyway, if you stick to this manual 256colors=true line then please update the other brand new skin that also uses 256 colors ;)

comment:26 Changed 6 years ago by andrew_b

  • Votes for changeset set to andrew_b

comment:27 Changed 6 years ago by andrew_b

  • Blocking 2173 added

comment:28 Changed 6 years ago by andrew_b

  • Blocking 2475 added

comment:29 Changed 6 years ago by slavazanko

  • Votes for changeset changed from andrew_b to slavazanko

comment:30 Changed 6 years ago by angel_il

  • Votes for changeset changed from slavazanko to slavazanko angel_il

comment:31 Changed 6 years ago by angel_il

  • severity changed from on review to approved

comment:32 Changed 6 years ago by slavazanko

  • Status changed from accepted to testing
  • Votes for changeset changed from slavazanko angel_il to commited-master
  • Resolution set to fixed
  • severity changed from approved to merged
  • Blocking 2173, 2475 removed

merge changeset:6ab2753063c3f538fd6a545681873445ea10156f

git log --pretty=oneline 1b99570..0f2578d
Last edited 5 years ago by andrew_b (previous) (diff)

comment:33 Changed 6 years ago by slavazanko

  • Status changed from testing to closed
Note: See TracTickets for help on using tickets.