fltk / fltk

FLTK - Fast Light Tool Kit - https://github.com/fltk/fltk - cross platform GUI development
https://www.fltk.org
Other
1.66k stars 269 forks source link

Fl_Tree doesn't use system colors #972

Closed CyprinusCarpio closed 3 months ago

CyprinusCarpio commented 5 months ago

Describe the bug Fl_Tree doesn't follow the scheme colors. The only correctly set color is the tree item selection color, others are not set. This can be seen best when the system uses a dark color scheme. On Windows, the FLTK colors are sourced from the classic theme colors (deprecated after Win7 but still functional as of Win11). On Linux the FLTK colors are sourced using some (presumably deprecated too) X mechanism, but it only works in the TDE and MATE desktop envinroments. Moreover, the line color appears to be hard coded, and the expando buttons are drawn from a XPM bitmap.

To Reproduce Considering that this behaviour can't be (visibly) reproduced on modern windows without modding the system substantially, it's best to test it on a Win7 virtual machine. Steps to reproduce the behavior:

  1. On a win7 VM, select the classic theme and apply some crazy dark colors.
  2. Run the demo tree program.

Expected behavior The Fl_Tree should set all the colors correctly, the line color should be derived from one of the FLTK colors, and the expando buttons need to follow the color scheme too.

Screenshots imgur

FLTK Version

FLTK Configure / Build Options All default CMake options, sans the disabled FLTK_BUILD_GL due to some unrelated problems. The problem reported persists regardless of this setting anyways.

Operating System / Platform:

Linux/Unix Runtime, if applicable:

Additional context Two things:

erco77 commented 5 months ago

Investigating.

You wrote:

select the classic theme and apply some crazy dark colors

What are the specific steps exactly to doing this? Would like to replicate exactly.

Also: when this dark theme is set, can you include a screenshot of what e.g. test/browser looks like with that theme applied? Fl_Tree is based a lot on Fl_Browser, so it would be good to see how different that looks.

erco77 commented 5 months ago

By the way, you can copy/paste images directly into github issues; imgur isn't needed.

CyprinusCarpio commented 5 months ago

select the classic theme and apply some crazy dark colors

What are the specific steps exactly to doing this? Would like to replicate exactly.

After the system is installed and booted to desktop, right click on the desktop for the context menu, then click on Personalize, then select the "High Contrast 1" theme. That'll switch to the classic theme and set the colors to dark.

Also: when this dark theme is set, can you include a screenshot of what e.g. test/browser looks like with that theme applied? Fl_Tree is based a lot on Fl_Browser, so it would be good to see how different that looks.

obraz

LGTM.

erco77 commented 5 months ago

Attaching a program to test the defaults of Fl_Tree and Fl_Browser here as tree-brow.cpp.txt just so we can compare apples to apples, as I think the test/tree program might be setting colors to suit itself that are not the default colors.

From what I can tell the default bg color and selection colors are the same for both widgets.

However, you did mention too the line drawing stuff, which I haven't checked yet, but I just want to first see if the bgcolor and selection colors are at least consistent.

From what I can tell, both Fl_Tree and Fl_Browser initialize by default with the same background and selection colors, for instance the attached program should look the same for both widgets, even with different themes.

CyprinusCarpio commented 5 months ago

It seems that the test/tree program does set it's colors explicitly, propably initializing to the default values that are set in the interface. So, it's not a good example to use for this issue. But some problems remain:

After adding argc and argv to your test program, the following is shown: obraz

On the other hand, examples/tree-simple has the correct text color: obraz

The line color is just a darker shade of FL_BACKGROUND2_COLOR.

erco77 commented 5 months ago

OK, good -- that's something I can start on.

My Win7 system can't have a compiler installed (because it's used for production testing applications work ok specifically on a system that doesn't have a compiler installed), but I should be able to build on e.g. Win10 and then test on the Win7 machine using the test you describe.

Will follow up.

erco77 commented 3 months ago

OK, so looking into this:

Looking at the default color map chart, the FL_BACKGROUND2_COLOR used for the tree background is highlighted with the top/right red box, and Fl_Color(43), used for drawing the lines, is highlighted with the center red box:

image

The default colors seem pretty far apart contrast-wise, ~50% difference.

So when the dark map is applied, it appears to be compressing the grayscale so far down to black that 50% is less than 1%. I'd really have to fault the color scheme for flattening that out waaay too much.

According to my color picker, the 50% gray for Fl_Color(43) (hex #808080 by default) is getting mashed down to near-black (hex #010102), and the background2 color is only slightly brighter (hex #121019).

I suppose if I can't depend on the grayscale at all, I could change the default connectorcolor() to be Fl_Color(15), the color immediately below Fl_Color(7) on the colormap chart. Not sure where that ends up when the dark map is applied, but not sure if that's a good idea anyway, as that color is also the FL_SELECTION_COLOR.

erco77 commented 3 months ago

Trying to find a better default for the connectorcolor().

Jumped on my Win7 machine and switched to Personalize -> High Contrast #1. _Gotta say, most of the FLTK test apps aren't navigable in that theme because there's so much black.. so it's not just FlTree that's looking bad here.

I ran the FLTK test program test/color_chooser and clicked fl_show_colormap() just to see how the default map (left) compares to when the high contrast #1 theme is applied (right):

image

So for sure most of the 24 color grayscale map (colors 32 ~ 55) is squashed to mostly black; the only thing resembling a grayscale ramp are the last 4 colors in the ramp, the rest is black. Weird; very extreme.

Anyway, if the grayscale ramp can't be trusted, the only other "system" line drawing color I could see working might be Fl_Color(8), which seems to be 33% brightness for both default and high contrast themes, which is very close to the intended default of 50%, and holes constrast with respect to the tree's default bg color, Fl_Color(7) in both theme cases.

So try setting connectorcolor(8) in your tree app and see if that works for you.

I'm not sure if it's a good idea for me to change the default, as existing apps may already depend on that, and I'm not sure the default Fl_Tree is using is "wrong". But for the default case, it seems like a subtle color change; the lines would only be a little bit darker than they are now.

CyprinusCarpio commented 3 months ago

connectorcolor(8) provides sufficient contrast for the connector lines, but the expando buttons still look kind of out of place and the text color is wrong in the test program. bez tytułu

#include <FL/Fl.H>
#include <FL/Fl_Double_Window.H>
#include <FL/Fl_Tree.H>

int main(int argc, char** argv) {
    Fl_Double_Window* win = new Fl_Double_Window(400, 400);
    Fl_Tree* tree = new Fl_Tree(0, 0, 400, 400);
    tree->connectorcolor(8);
    tree->add("Line one");
    tree->add("Line two");
    tree->add("Line three");
    tree->add("Line three/Subline one");
    tree->add("Line four");
    tree->add("Line four/Subline one");
    tree->add("Line four/Subline two");
    win->end();
    win->resizable(win);
    win->show(argc, argv);
    return Fl::run();
}
Albrecht-S commented 3 months ago

@erco77 FYI: the "Gray Ramp" - as it is called - is calculated dynamically when setting up the program's color scheme/theme, or more exactly: when calling Fl::background(...) which is called when reading the background color from the system by system specific means.

There are some issues with this approach, the most important one being the first:

  1. This calculation was designed to be used with bright (or light) background colors.
  2. The calculation itself is flawed, it uses an algorithm that has nearly nothing to do with human contrast perception.

To shed some light on this issue I wrote a test program (gray_ramp.cxx) that

Example Screenshot: image

The current background color FL_BACKGROUND_COLOR is the one with the "down box" in the colormap (index 49 or fl_gray_ramp(17)).

Example output:

Background color = (31, 31, 31), fl_lightness =  15.00
fl_gray_ramp( 0) = (00, 00, 00), fl_lightness =   0.00, delta =   0.00
fl_gray_ramp( 1) = (00, 00, 00), fl_lightness =   0.00, delta =   0.00
fl_gray_ramp( 2) = (00, 00, 00), fl_lightness =   0.00, delta =   0.00
fl_gray_ramp( 3) = (00, 00, 00), fl_lightness =   0.00, delta =   0.00
fl_gray_ramp( 4) = (00, 00, 00), fl_lightness =   0.00, delta =   0.00
fl_gray_ramp( 5) = (00, 00, 00), fl_lightness =   0.00, delta =   0.00
fl_gray_ramp( 6) = (00, 00, 00), fl_lightness =   0.00, delta =   0.00
fl_gray_ramp( 7) = (00, 00, 00), fl_lightness =   0.00, delta =   0.00
fl_gray_ramp( 8) = (01, 01, 01), fl_lightness =   0.00, delta =   0.00
fl_gray_ramp( 9) = (02, 02, 02), fl_lightness =   0.01, delta =   0.01
fl_gray_ramp(10) = (03, 03, 03), fl_lightness =   0.02, delta =   0.01
fl_gray_ramp(11) = (05, 05, 05), fl_lightness =   0.07, delta =   0.05
fl_gray_ramp(12) = (07, 07, 07), fl_lightness =   0.16, delta =   0.09
fl_gray_ramp(13) = (0b, 0b, 0b), fl_lightness =   0.48, delta =   0.32
fl_gray_ramp(14) = (11, 11, 11), fl_lightness =   1.36, delta =   0.88
fl_gray_ramp(15) = (19, 19, 19), fl_lightness =   3.43, delta =   2.07
fl_gray_ramp(16) = (23, 23, 23), fl_lightness =   7.69, delta =   4.26
fl_gray_ramp(17) = (31, 31, 31), fl_lightness =  15.00, delta =   7.31 <- FL_BACKGROUND_COLOR
fl_gray_ramp(18) = (43, 43, 43), fl_lightness =  23.82, delta =   8.82
fl_gray_ramp(19) = (5a, 5a, 5a), fl_lightness =  34.42, delta =  10.60
fl_gray_ramp(20) = (77, 77, 77), fl_lightness =  47.05, delta =  12.62
fl_gray_ramp(21) = (9b, 9b, 9b), fl_lightness =  61.89, delta =  14.84
fl_gray_ramp(22) = (c8, c8, c8), fl_lightness =  79.51, delta =  17.62
fl_gray_ramp(23) = (ff, ff, ff), fl_lightness = 100.00, delta =  20.49

I learned a lot about human contrast perception when working on the new fl_contrast() function. The term "lightness" above is - as documented here - "almost linear with respect to human perception". This means the gray ramp should ideally have an almost constant DELTA value between two successive colors - which is definitely not the case. This explains why you see only a few usable colors at the end of the gray ramp when using a dark theme.

This all can't be changed in FLTK 1.4.x because any change in calculating the gray ramp might break user programs in unforeseeable ways, making text unreadable etc..

I believe that we should support "dark mode" in FLTK 1.5 though. This would be the time to rethink and re-implement the gray ramp - or add other means for selecting background colors etc..

Coming back to this issue: I believe that you need to use fl_contrast() wherever a {system or user} selectable color is used. This applies to the connector color as well as all text and label colors.

erco77 commented 3 months ago

@Albrecht-S I suppose if fl_contrast() works better now than it used to, then I'll try using it. It didn't used to work very well; I recall numerous problems from early on and throughout, so I quickly avoided using it during early development. I know you spent a bit of time on it recently (last few years), so it might be worth revisiting here for the tree's "connection lines", using a contrast relative to the background color. I'll give it a try.

@CyprinusCarpio Ya, that issue with the open/close icons is /probably/ because by default they're drawn as XPM images, which necessarily use fixed 24 bit hex colors, i.e.

const char * const Fl_System_Driver::tree_open_xpm[] = {
  "11 11 3 1",
  ".    c #fefefe",  // <-- ICON BG
  "#    c #444444",  // <-- BOX OUTLINE
  "@    c #000000",  // <-- +/- SYMBOL COLOR
  "###########",
  "#.........#",
  "#.........#",
  "#....@....#",
  "#....@....#",
  "#..@@@@@..#",
  "#....@....#",
  "#....@....#",
  "#.........#",
  "#.........#",
  "###########"
};

I'm not sure, but I don't think our XPMs support color map colors. (I might be wrong on this; I suppose if 32bit values are allowed, then we /could/ specify colormap colors as e.g. #000000ff)

Since you can set the openicon() + closeicon() values to be any Fl_Image based image, I suppose you could craft a PNG that uses an alpha channel image that might be more compatible with color theme changes. Or perhaps even an SVG image, where maybe actual shape drawing code can be used, and possible color map colors. So you might want to try that.

The only thing I can think of that could help the default case would be if I changed the behavior of setting openicon() and closedicon() to NULL, then it would fall back to /drawing/ the icons (with the usual FLTK drawing functions) instead of using the default XPM, which would make Fl_Tree's default behavior more compatible with color themes.

Albrecht-S commented 3 months ago

I suppose if fl_contrast() works better now than it used to, then I'll try using it.

@erco77 Please try this modified demo program as a proof of concept:

#include <FL/Fl.H>
#include <FL/Fl_Double_Window.H>
#include <FL/Fl_Tree.H>

int main(int argc, char** argv) {
    Fl_Double_Window* win = new Fl_Double_Window(400, 400);
    Fl_Tree* tree = new Fl_Tree(0, 0, 400, 400);
    win->end();
    win->resizable(win);
    win->show(argc, argv);

    tree->connectorcolor(fl_contrast(fl_darker(FL_RED), FL_BACKGROUND2_COLOR));
    tree->item_labelfgcolor(fl_contrast(FL_BLUE, FL_BACKGROUND2_COLOR));

    tree->add("Line one");
    tree->add("Line two");
    tree->add("Line three");
    tree->add("Line three/Subline one");
    tree->add("Line four");
    tree->add("Line four/Subline one");
    tree->add("Line four/Subline two");

    return Fl::run();
}

Notes:

Is there a better way to set the labelcolors later (w/o touching every single item)?

Albrecht-S commented 3 months ago

One remaining problem can certainly be that you don't know the real background colors at construction time, as can be seen in the demo program posted by me - because the user may set them later when setting up a color theme or even switching from light mode to dark mode at runtime. You should keep in mind that nothing is sure at Fl_Tree's construction time, neither the FLTK scheme nor any background colors.

erco77 commented 3 months ago

@Albrecht-S As an aside: perhaps fl_contrast() should be added to the Colors section of the "Drawing things in FLTK" part of the manual, as it seems notably absent.

Albrecht-S commented 3 months ago

perhaps fl_contrast() should be added to the Colors section of the "Drawing things in FLTK" part of the manual

@erco77 Thanks, I'll take a look.

erco77 commented 3 months ago

@CyprinusCarpio Regarding the text color looking wrong, hmm.. when I compile your program and run it with the Personalize -> High Contrast #1, I get this result, where the text color is very readable (yellow), just like all the other test programs:

image

This doesn't look like your result at all -- can you double-check your test?

Fl_Tree sets the text color (or labelfgcolor() ) to FL_FOREGROUND_COLOR (Fl_Color(0)) which seems correct and consistent with the rest of what FLTK uses.

CyprinusCarpio commented 3 months ago

Shit... My bad. The CMake cache of my test project had the FLTK_DIR set to the vcpkg one (1.3.8). I could swear I changed it for all my FLTK projects and removed the vpckg install... I've now changed it to where I keep the 1.4.0 version. This also explains why the text color was wrong in the test programs I compiled myself (unknowingly using the vpckg version) but correct in the test programs from the repo that I compiled. So, everything works as intended, the only thing is the buttons, but that's not a bug per se.

erco77 commented 3 months ago

OK, I think the one thing might be to change connectorcolor() to either use Fl_Color(8), or perhaps fl_contrast() if absolutely necessary.

_For the latter, might have to work with Albrecht on what the right usage would be, as I'm not sure I fully understand how fl_contrast() should be used here, specifically what to set the 'fg' value to be. My goal is something "lighter" or "less contrast" than the text color._

CyprinusCarpio commented 3 months ago

I removed the XPM button and implemented the drawing in Fl_System_Driver. The button frame is drawn with Fl_Color(8), and the symbol with FL_BACKGROUND2_COLOR. The button is of the same size as the XPM one and is positioned the same. The line color is changed to Fl_Color(8). bez tytułu PR to follow one these days after I test that nothing is broken.

Albrecht-S commented 3 months ago

I'm not sure I fully understand how fl_contrast() should be used here, specifically what to set the 'fg' value to be. My goal is something "lighter" or "less contrast" than the text color.

That's easy: set fg to the value that is intended to be used and bg to the background color you want to draw on. However, if this color doesn't have sufficient contrast with the background then it will be "converted" to either black or white. As you suggested I added an overview of fl_contrast() and friends to the chapter "Drawing Things" in the FLTK docs.

I uploaded this already, see https://www.fltk.org/doc-1.4/drawing.html#drawing_contrast

Simple example from the docs:

Fl_Color bg = color();          // background color of the widget
Fl_Color fg = FL_BLUE;          // the chosen foreground (drawing) color
fl_color(fl_contrast(fg, bg));  // set the drawing color
fl_rect(..);                    // draw a rectangle with sufficient contrast

Caveat: if you draw on a light background and the text color is some kind of gray, then you might (theoretically) use

fl_color(fl_contrast(TEXT_COLOR, bg));
// draw text ...
fl_color(fl_contrast(fl_lighter(TEXT_COLOR), bg));
// draw connector lines

Assuming that TEXT_COLOR would be the text or label color and the connector lines shall have less contrast (therefore fl_lighter()). However, it might happen that TEXT_COLOR is used as-is and the lighter color set for the connector lines would become FL_BLACK (i.e. darker) - which is the opposite of what you wanted to achieve.

There's no easy and general rule to find out which color would give the intended result, particularly if the user can set the colors! The fl_lightness() function used in my demo program could be used to get the lightness values of all involved colors and calculate the contrast (by simple subtraction) but that's probably too much to do in the general case of such a widget. Everything would also depend on the dark mode or light mode of the application (i.e. dark background or light background). Not an easy job. fl_contrast() would always give at least a "readable" contrast w/o dark gray text on black background.

Conclusion:

The result would be: if the user selects proper colors with sufficient contrast they get what they want. If not, the result will always be at least "readable". That's it.

Albrecht-S commented 3 months ago

PS: as written before, please don't try to set up colors depending on the system scheme or any background colors at widget construction time. Instead use fl_contrast() at drawing time because the background color, the "theme" (day or night), and other color settings can be changed during runtime. It's just not worth to try to "do it right" for everything!

erco77 commented 3 months ago

@Albrecht-S Perhaps let's open a new/separate issue if you think it's worth exploring changing Fl_Tree (and Fl_Table, etc) to make better use of fl_contrast() during draw().

I feel like I've been through this washing machine before: where algorithmic control of colors at draw() time may help in the presence of flexible color schemes, but drives application programmers to madness when they try to carefully curate colors with subtle contrasts, only to have their precise color choices trumped by something like fl_contrast(). Certainly the key to making "pretty" interfaces "easy on the eyes" is avoiding high contrasts in text and widget rendering, which "myspace" pages and old 1990's applications were so famously guilty of.

Question: does fl_contrast() limit itself to color map colors only? I'm thinking an application programmer trying to carefully design their colors would set colors to absolute RGB values, and if so, would hopefully avoid any algorithmic modification of their color choices. If that's the case, I'd feel better about using fl_contrast() in draw(). But if it affects RGB colors too, that could be a problem IMHO. I can't really tell from the fl_contrast() docs if it only works specifically to colormap colors, or affects RGB values too.

Folks are generally used to FLTK's warts, and have found ways to work around. In my own commercial apps I let the user redefine color schemes at runtime with the click of a button in a "color themes" menu, and I simply rebuild all the widgets/menus whenever that happens. It took some redesign of my app, but it works and actually made the app better in some ways. So it'd be bad if we worked-around their work-arounds.

I think for now I might take the easy route of changing the connectorcolor() to 8, and be done for this issue. Apparently my choice of using the "middle" of the grayscale was a bad one if non-default color schemes use some kind of steep curve where "the middle" is still almost black. My understanding has been the grayscale spans the three rows in the color chip chart 32 ~ 55, and by default there's a nice linear ramp there. But when show(argc,argv) is used, "system colors" are applied instead, those rules seem different; comparing the fltk default linear ramp (left) to your gray_ramp() example (right):

image

..those are so far apart, they can't really be relied upon, which seems to defeat the whole purpose.. yikes!

Albrecht-S commented 3 months ago

@erco77 I'm trying to answer some of your questions as quick as I can because it's too late here...

  1. Yes, we can open another issue for exploring usage of fl_contrast() in Fl_Tree and Fl_Table, but I won't have the time to do it before the 1.4 release. It's something I would like to do when 1.5 starts development. This would also be the time when we think about FLTK color "themes" (as opposed to "schemes"), particularly dark mode etc.. Your own themes might be very helpful but I also know of another source...

  2. There's a big difference whether you build an entire application - maybe with different color "themes" - or you design a single widget like Fl_Tree or Fl_Table intended to work in arbitrary system environments. If you write an app that supports color themes you may not need to use system colors, and then everything you design can look as you designed it, no surprises. I'm thinking here of prodatum as well as your own application(s).

  3. fl_contrast() works on RGB colors. If you provide it with an indexed FLTK color it picks its RGB value and calculates the contrast between fg and bg. There's no difference. The return value is one of three values: fg, FL_BLACK, or FL_WHITE where fg is returned if the contrast is sufficient, and black or white only if not. In other words: if fg is an indexed color (one from the colormap), then so will be the output. Another aspect: if you input valid colors with sufficient contrast, then fl_contrast() is a no-op because it returns fg. The result of fl_contrast() with the new algorithm in 1.4 is almost the same as in 1.3 but border cases where the old algorithm allowed insufficient contrast are now much less likely (they should never happen). Hence, the readability of text and the distinction of graphics should be much improved.

Note: I considered another option to make the fg color darker or brighter until the minimum contrast is reached but this is not implemented. This would be another fl_contrast_mode() in the future (if at all).

  1. I'm fine with "I [might] take the easy route of changing the connectorcolor() to 8, and be done for this issue". If this works better for most (or at least some) cases it's an improvement and I second it.

More technical details to the so-called "gray ramp":

You wrote:

My understanding has been the grayscale spans the three rows in the color chip chart 32 ~ 55, and by default there's a nice linear ramp there. But when show(argc,argv) is used, "system colors" are applied instead, those rules seem different;

That would be my understanding as well, but in fact it's not the case. I consider this a bug, but as long as we don't have a clear definition of the "gray ramp" it's a problem. The docs say it's a "gray ramp" but it can be any color. And more, the algorithm that generates the ramp is wrong and produces a kind of exponential rather than a linear ramp (WRT to lightness, see output of the gray_ramp demo). It (kind of) "works" for the old "battleship gray" color categories but not for something like your tan color theme would need (if you didn't initialize all necessary colors yourself).

Here's a random non-gray example (./gray_ramp -bg '#edb') (left side) and one if you use a darker background color (./gray_ramp -bg '#642') (right side): image

So yes, there's definitely no "middle gray" color to be expected in the middle of the "gray ramp". However this is IMHO nothing we can change in 1.4.x but something we should consider when we start with 1.5.x development.

Albrecht-S commented 3 months ago

BTW: instead of using numerical color values using symbolic names would be more appropriate [¹], see FL/Enumerations.H:

  const Fl_Color FL_FOREGROUND_COLOR = 0 // ... used for labels and text 
  const Fl_Color FL_INACTIVE_COLOR = 8   // the inactive foreground color 

¹) @erco77 I'm not saying that FlTree currently uses numerical color values. I mention it because you wrote "changing the connectorcolor() to 8"_ in your comment above. I also noticed it in @CyprinusCarpio's PR #995. Using (and documenting) that the default connector color is FL_INACTIVE_COLOR would IMHO be fine.