Updated scintilla_disable_color_palette() and support default colors#40
Updated scintilla_disable_color_palette() and support default colors#40rhaberkorn wants to merge 2 commits into
Conversation
This tries to preserve the 16 default colors if the terminal doesn't have enough colors.
allows substituting specific foreground and background colors with ncurses default colors (-1), which will preserve the terminal emulator's native color scheme.
|
Is api.md somehow generated? |
|
Yes, api.md is generated by |
|
@orbitalquark btw. I noticed that Scinterm 6.0 does no longer compile with older Scintilla 5.5.x releases, probably due to b3d20c9. |
|
Yes, I've already made a note in the "Requirements" section on the main page/README that Scinterm 6.x requires Scintilla 5.6.2 or greater. |
|
To answer the question "do we really want to support default colors; is it worth it?" My first instinct is "no". This sounds too complicated for a minuscule subset of users that would benefit from this. I am not confident I could maintain this feature going forward because I honestly cannot even comprehend it right now, despite your nice write up. |
It's not a miniscule amount of users, really. Lots of terminal emulators allow changing the color scheme. GNOME Terminal is Ubuntu's default terminal emulator. xterm also has a black-on-white scheme by default if I am not mistaken. The Haiku terminal is black-on-white by default. I remember seeing OS X terminals as well with bright schemes. And some terminal text editors do make use of default colors (vim, nano, etc.) and work seamlessly with any color scheme your terminal dictates. I'll try to explain it with a different program: |
That was for me the real motivation to look into default colors. SciTECO looked ugly on GNOME Terminal's default settings, while other editors didn't. But arguably if your user has to enable some setting manually, you could also tell him to edit the emulator's color palette and make COLOR_BLACK a real black. |
orbitalquark
left a comment
There was a problem hiding this comment.
Thanks for the additional info on default colors. I appreciate you taking the time to help me understand. I'm very slowly coming around to getting the picture in my head.
|
|
||
| --- Sets the offsets for colors and color pairs generated on-demand. | ||
| -- Applications that define their own colors and color pairs can tell Scinterm where to start from. | ||
| -- This should be called after `start_color()`. |
There was a problem hiding this comment.
Scinterm calls start_color() from the Colors::Colors() constructor. Since scintilla_set_color_offsets() invokes the singleton instance of Colors(), I don't think this comment is necessary.
| -- when used as the given foreground or background color. | ||
| -- Default colors might depend on the emulator's color scheme. | ||
| -- This might not be supported in all curses implementations. | ||
| -- You must still call first `start_color()`, then `use_default_colors()` |
There was a problem hiding this comment.
Same comment as above: scintilla_set_default_colors() invokes the singleton instance of Colors(), which calls start_color(). I think we only need to call out the need for use_default_colors() or assume_default_colors() here.
| if (colorOffset + colors.size() >= std::numeric_limits<short>::max()) return COLOR_WHITE; | ||
| const short c = colorOffset + colors.size(); | ||
| init_color(c, color.GetRed() * 1000.0 / 255, color.GetGreen() * 1000.0 / 255, | ||
| short i = instance().colorOffset + instance().colors.size(); |
There was a problem hiding this comment.
This can overflow if the right-hand-side is greater than short max, right?
| if (!instance().usePalette && COLORS > 16 && instance().colorOffset < 16) | ||
| i += 16 - instance().colorOffset; |
There was a problem hiding this comment.
I don't understand this. Can it be simplified? My branch had if (!usePalette) i += std::clamp(COLORS, 8, 16); Remember the next line short-circuits an i that exceeds COLORS.
|
|
||
| short Colors::Pair(const ColourRGBA &fore, const ColourRGBA &back) { | ||
| if (!has_colors()) return 0; | ||
| short Colors::Pair(attr_t &attrs, const ColourRGBA &fore, const ColourRGBA &back) { |
There was a problem hiding this comment.
No, I don't like the silent pass through and modification of an attrs parameter. For me, this will cause hard-to-debug issues. I feel like I either brought this up in another PR, or we discussed this back when Colors::Pair() returned an attr_t instead of a color pair number.
We should probably have a private function that returns WA_REVERSE that we can | to an existing attr_t before calls to Colors::Pair() and mvwchgat() and wattr_set().
| void Colors::SetDefaultColors(int fore, int back) { | ||
| instance().defaultFore = fore; | ||
| instance().defaultBack = back; | ||
| } | ||
|
|
||
| int Colors::GetDefaultFore() { | ||
| return instance().defaultFore; | ||
| } | ||
|
|
||
| int Colors::GetDefaultBack() { | ||
| return instance().defaultBack; | ||
| } |
There was a problem hiding this comment.
I think we can have static DefaultFore and DefaultBack members just like the colors Black, Red, etc. Since they're not const, they should be mutable for Colors::SetDefaultColors().
Maybe we can store it as a curses color number and ColorRGBA pair so we don't have to keep doing lookups later and also keep Colors::get() as a private instance method.
| // Normally this clears the given portion of the screen with the given background color. In | ||
| // some cases however, it can be determined that whitespace is being drawn. If so, draw it | ||
| // appropriately instead of clearing the given portion of the screen. | ||
| // This can also draw the caret. |
There was a problem hiding this comment.
Thanks for pointing this out.
| static void SetDefaultColors(int fore, int back); | ||
| /** Gets default foreground color as an integer RGB */ | ||
| static int GetDefaultFore(); | ||
| /** Gets default background color as an integer RGB */ | ||
| static int GetDefaultBack(); |
There was a problem hiding this comment.
I don't like the storage class here because I keep thinking it's a curses color number, not an 0xBBGGRR int. Users may or may not get confused that this needs to be 0xBBGGRR instead of 0xRRGGBB. There might be room for improvement here.


scintilla_disable_color_palette()
The first part is an update to your scintilla_disable_color_palette(): This preserves the first 16 colors only if usePalette is disabled (otherwise the palette colors are already in the colors table) and only if and as much as the number of colors allows. For instance, on linux-16color you will have only 16 colors, but can initialize new ones (can_change capability).
I hope this can replace #34. I can of course split this into two PRs if you like.
scintilla_set_default_colors()
The second part will be much more debatable. It adds support for "default colors". Default colors are the colors of your terminal if you don't change the foreground or background colors, ie. they are the terminal's native "color scheme". For instance many terminal emulators support both white-on-black and black-on-white schemes without completely redefining the 16 palettized colors. In fact the palette's colors may not and often don't overlap with the default colors. On ncurses you get default colors either by using color pair 0, or by calling the ncurses-only use_default_colors(). In the latter case you can use color id -1 as either the foreground or background color in a pair, representing the "native" "default" foreground or background color.
Supporting this is important if you want to write applications that work with all terminal color schemes without breaking them. E.g. if you have a black-on-white color scheme, you would expect your editor to be black-on-white as well instead of switching to white-on-black. vi, nano etc. support that as well.
For instance, here are screenshots of SciTECO taken only by changing GNOME Terminal's color scheme without any restart:
In order to achieve this, scintilla_set_default_colors() allows you to define Scintilla 0xBBGGRR colors that act as placeholders for the default color (-1) when used in the foreground or background.
This patch got a lot more difficult than I anticipated because
But...
A big disadvantage of default colors is that you are effectively restricted to only the default colors, their reverse (via WA_REVERSE) and colors that are expected to work more or less well in all terminal emulator color schemes (e.g. red foreground, default background). For instance, if you try to make your block caret bright white (0xFFFFFF) to stand off better, you have already lost - since this is not the default color, your caret would be practically invisible on black-on-white schemes. That's why I made SciTECO use default colors only in its very conservative default settings and had to make them even more conservative than they were already.
However there would be one use of scintilla_set_default_colors() on more fancy application-defined color schemes as well: You might have noticed that some brain-dead terminal emulators (GNOME Terminal, but not only) makes a difference between its default background and COLOR_BLACK even with "Black on white" default colors. In the shell you have a proper deep black background, but once you start your application it has an ugly washed-out greyish-black background. This is because your app requested COLOR_BLACK and the black entry from the terminal's palette is not identical to the default background. In this case you could call
scintilla_set_default_colors(-1, 0x000000);to use the default background (-1) instead of 0x000000 (which would be translated to COLOR_BLACK unless you also called scintilla_disable_color_palette()). On the down side, such a setting would practically break your app on all non-bright-on-dark builtin terminal schemes and you cannot check for any of this automatically. This would have to be a setting made by your user who knows exactly how his terminal emulator is configured.The first question is, do we really want to support default colors, is it worth it?
Terminals and terminal emulation are a mess.
PS: I cannot guarantee that the "default color" patch didn't cause subtle regressions.