Ticket #3711 (closed enhancement: fixed)

Opened 5 months ago

Last modified 4 months ago

Color aliases

Reported by: egmont Owned by: andrew_b
Priority: minor Milestone: 4.8.19
Component: mc-skin Version: master
Keywords: Cc:
Blocked By: Blocking: #3724
Branch state: merged Votes for changeset: committed-master

Description

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!

Attachments

mc-3711-color-aliases.patch (10.8 KB) - added by egmont 5 months ago.
mc-3711-color-aliases-v2.patch (11.9 KB) - added by egmont 5 months ago.

Change History

Changed 5 months ago by egmont

comment:1 Changed 5 months ago by 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.)

comment:2 Changed 5 months ago by 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?

comment:3 follow-up: ↓ 4 Changed 5 months ago by 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?

comment:4 in reply to: ↑ 3 ; follow-up: ↓ 6 Changed 5 months ago by 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?

comment:5 Changed 5 months ago by 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.

comment:6 in reply to: ↑ 4 ; follow-up: ↓ 7 Changed 5 months ago by 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.)

comment:7 in reply to: ↑ 6 Changed 5 months ago by 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.

comment:8 Changed 5 months ago by 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...

Changed 5 months ago by egmont

comment:10 Changed 5 months ago by 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 :) but still independent from the current issue.

Last edited 5 months ago by egmont (previous) (diff)

comment:11 Changed 5 months ago by andrew_b

  • Blocking 3724 added

comment:12 Changed 4 months ago by andrew_b

  • Status changed from new to accepted
  • Owner set to andrew_b
  • Votes for changeset set to andrew_b
  • Branch state changed from no branch to on review
  • Milestone changed from Future Releases to 4.8.19

Branch: 3711_skin_color_aliases
Initial changeset:b6207f218f4eff3f4108d3fc831c9e7d6aa0ab79

comment:13 Changed 4 months ago by andrew_b

  • Branch state changed from on review to approved

comment:14 Changed 4 months ago by andrew_b

  • Status changed from accepted to testing
  • Votes for changeset changed from andrew_b to committed-master
  • Resolution set to fixed
  • Branch state changed from approved to merged

Merged to master: [5662ad6d741a80f56498e30a3f413a6211d2c4c6].

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

comment:15 Changed 4 months ago by andrew_b

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