Color aliases

[egmont]

Skins (and especially a set of skins that are similar to each other) are really cumbersome to create for a variety of reasons.

One of them is that the most prominent colors of a skin have to appear multiple times in the file. Sometimes it's the very same color pair repeated, but more often it's either the same background combined with a different foreground, or the other way around. Also, some of the main colors of a skin might appear at both positions (see e.g. highlighted text, or the badly named "reverse" keyword).

Skin definitions really cry for something like the #define preprocessor directive (but we're doomed with this since there's no convention for this in .ini files and we're using parser methods that don't support these) or an explicit section where aliases can be defined (this is what I implemented).

So e.g. instead of this:

[core]
    _default_ = white;blue
    selected = yellow;blue
    marked = white;cyan
    markselect = yellow;cyan

from now on you can write:

[aliases]
    bgmain = blue
    bghilite = cyan
    fgmain = white
    fgselect = yellow
[core]
    _default_ = fgmain;bgmain
    selected = fgselect;bgmain
    marked = fgmain;bghilite
    markselect = fgselect;bghilite

Advantages of the new approach:

• You only modify the actual color at one single place.

• No search-and-replace madness with e.g. unwanted sub-word matches, or replacing only from the cursor location and not in the entire file.

• If two different roles accidentally happen to have the same color but you do not necessarily want it this way, you can define two different aliases, and changing just one of them won't change the other one as a search-and-replace would do.

• It's way easier to diff two variants of a skin, or two versions as you develop one presumably in some VCS.

Patch attached that implements this feature, and modifies the two "gray-*" skins.

Run "diff -u gray-green-purple256.ini gray-orange-blue256.ini" before applying the patch, and run it after applying it. It's quite self-explanatory. After applying the patch, the only difference between the two files is the two alias definitions.

Please comment, review etc... thanks!

[mooffie]

(BTW, I've just opened #3712, which isn't quite related to this ticket. It isn't an attempt to derail your ticket; on the contrary. Somebody is liable to bring up the editor's colors issue sooner or later, so it's best to direct that traffic to the proper ticket.)

[egmont]

@mooffie: Do you think that this patch goes against that goal, or does it go for it?

I think (I hope) that it goes mostly in that direction. Nitpicking: I'm pondering however that if the scope of color definitions will not be limited to a single file but will have effect across several files then maybe "[definitions]" would probably be better section name than "[aliases]". What do you think?

[mooffie]

Replying to egmont:

@mooffie: Do you think that this patch goes against that goal, or does it go for it?

It goes for it.

That question is indeed why I opened that other ticket: so we can make some assessment on whether this ticket introduces something that will have to be undone in the future (leading, for example, to something awful like having to support two syntaxes for the skin files, for backward-compatibility).

But I'd say that this ticket/patch seems safe enough.

maybe "[definitions]" would probably be better section name than "[aliases]". What do you think?

It's funny, I had a similar thought initially. I really don't know. I don't think it's important enough to lose sleep over ;-)

I'd suggest that maybe we learn slightly more (just slightly) on how #3712 will look like before we commit this patch.

--

Some potential nitpicking:

Should we start aliases with "$" or "%" so they stand out from the text (and can be syntax highlighted)? Or maybe we should use UPPERCASE (as I did in #3712)? Should we "standardize" on one style for both tickets?

[egmont]

Replying to mooffie:

Should we start aliases with "$" or "%" so they stand out from the text (and can be syntax highlighted)? Or maybe we should use UPPERCASE (as I did in #3712)?

Haha, I was also pondering about the same. I ended up not doing so, for no particular reason (I just liked it a bit better this way, but I can easily be convinced otherwise).

There are two possible approaches: Either strictly require one of these solutions (and e.g. the "$" sign can be used to refer to such a color, but not when being defined), or we could leave the code as-is and start using "$" or "%" or uppercase as a convention only. Which one is your preferred approach?

[egmont]

Now that I started to create a few similar (but not as similar as the gray* or the modarin*) skins, I just figured out it really sucks that an alias can't refer to another alias in the current patch.

E.g. in one of them I want two colors (e.g. "marked" and "header") to be the same even as I keep adjusting the exact RGB, whereas in another skin I want them to be decoupled; yet I'd prefer the skin files to only differ in their "[aliases]" section.

I'm planning to fix this similarly to how the Linux kernel handles symlinks and detects loops. It does not actually detect loops, there's just a reasonably low limit on the number of hops. Maybe I'll follow aliases at most 10 times per color lookup.

[mooffie]

Replying to egmont:

Replying to mooffie:

Should we start aliases with "$" or "%" so they stand out from the text (and can be syntax highlighted)? Or maybe we should use UPPERCASE (as I did in #3712)?

Haha, I was also pondering about the same. I ended up not doing so, for no particular reason (I just liked it a bit better this way, but I can easily be convinced otherwise).

There are two possible approaches: Either strictly require one of these solutions (and e.g. the "$" sign can be used to refer to such a color, but not when being defined), or we could leave the code as-is and start using "$" or "%" or uppercase as a convention only. Which one is your preferred approach?

I think we should do this by convention, at least for the time being. (At this early stage we don't quite know what we'll need so it isn't wise to put restrictions in the code.)

(Making the code enforce an "$" or "%" has an advantage though: the code can be optimized (as in: "if there's no '$' in the string, return"). Otherwise the subject string has to be broken into words and checked word by word. It's this tokenization that I'm worried about: next week I'll see if #3712 will need some special treatment in this regard.)

[egmont]

Replying to mooffie:

I think we should do this by convention, at least for the time being.

Sounds good. (In the mean time I started to use CamelCase for aliases in my work-in-progress skins.)

Making the code enforce an "$" or "%" has an advantage though: the code can be optimized

This could save a few CPU cycles while reading the ini files. Such micro optimizations don't make any sense and are also targeted at the wrong place (somewhere where speed isn't critical at all). Code cleanliness, readability, maintainability; user-facing behavior etc. are way stronger factors. If we decide to require a "$" or "%" prefix, it should be driven by some actual behavioral rationale, rather than saving a few nanoseconds.

I'm attaching a 2nd version of the patch where aliases can refer to other aliases (which can yet again refer to other aliases, and so on, as long as there's no loop) with no limit on the number of hops. Loops are detected of course so mc won't hang on a faulty skin.

[zaytsev]

Just to chime in here, since it seems that major changes are coming to skins, maybe it is reasonable to deal with #3159 as well in the same stream...

[zaytsev]

@egmont, for your interest:

​https://github.com/MidnightCommander/mc/commit/23505b76e174cf8b17cc5bcbd3a3d45d48515b9b

[egmont]

@zaytsev thanks for the link, I've updated the patch around the assertions.

major changes are coming to skins

This is a pretty minor optional new feature, and completely orthogonal to #3159. At this point I'm not planning to work on the latter.

In the mean time, I got hit by #3160 again while working on my new skins. This one would be nice to fix :)

[andrew_b]

Branch: 3711_skin_color_aliases
Initial changeset:b6207f218f4eff3f4108d3fc831c9e7d6aa0ab79

[andrew_b]

Merged to master: [5662ad6d741a80f56498e30a3f413a6211d2c4c6].

git log --pretty=oneline fc3d153..5662ad6