Color aliases #3711
Comments
|
(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.) |
@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? |
Replying to egmont:
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.
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? |
Replying to mooffie:
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? |
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. |
Replying to egmont:
(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.) |
Replying to mooffie:
Sounds good. (In the mean time I started to use CamelCase for aliases in my work-in-progress skins.)
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 thanks for the link, I've updated the patch around the assertions.
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. |
Branch: 3711_skin_color_aliases |
|
Merged to master: [5662ad6]. |
|
Important
This issue was migrated from Trac:
egmont(@egmontkob)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:
from now on you can write:
Advantages of the new approach:
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!
Note
Original attachments:
egmont(@egmontkob) onNov 2, 2016 at 23:19 UTCegmont(@egmontkob) onNov 5, 2016 at 10:11 UTCThe text was updated successfully, but these errors were encountered: