Fixes to respect bright colors in palette

[echuber2]

Fixes #133

It seems there are two errors in the existing logic in style.py:

1. When a palette defines the upper 8 colors (the "bright" colors), the existing code overwrites them anyway using the results of intensify.
2. When a palette does not define the upper 8 colors, the existing code does not define replacements for them in the 256-color palette rule families.

In other words, the code was overwriting some of the explicitly defined colors, but also failing to provide automatically brightened colors when it could have :-)

Note that with this fix in place, the existing test suite doesn't "pass" because the output colors will be different. For some users, this may be an unexpected change in behavior. In that case, maybe the old behavior should be preserved as a default while the new behavior is toggled optionally.

The code could be optimized to not make so many redundant calls to intensify, but I haven't bothered to make that change here.

[echuber2]

Now that the code respects the explicit definitions of the upper 8 colors, it was necessary to remove the "*2" on the osx 8-color palette so that its bright set will still be generated.

[echuber2]

@ssbarnea Can you run the CI checks on this? It failed linting before but I didn't check the log before the log expired, so I'm not sure what's wrong.

[ssbarnea]

Sure, just ping me when needed.

[ssbarnea]

@echuber2 I send you an invite so you do not need approval to run CI. I hope you can at least partially help with #135

[echuber2]

@ssbarnea Hi, so, it appears that because this PR effectively changes the color palette that is generated, the unit tests are going to fail unless they are also changed accordingly. Would you or someone else like to proofread what I've done here before I try to change the tests themselves? Thanks.

[ziegenberg]

Looks good to me.

[echuber2]

Nice, thanks. I thought perhaps I should generate before/after images to demonstrate that it's working, but I haven't found the time to do that yet.

[ssbarnea]

I am going to close this because I did not find any attempts to bring it to ready for review state. If things change I am more than happy to reopen it. Others are also welcomed to attempt to get the incomplete code and fix it.

[echuber2]

The code isn't incomplete. It's the old unit tests that need fixing as they are based on the old (broken) code. I don't have time to manually create the fixed tests right now but I am hoping that with the branch closed, it will not be overlooked or deleted in the future before someone finishes merging it.

[ssbarnea]

Is failing most CI jobs, which counts as... not complete. You want to create a feature branch directly inside the project?

I will reopen it, hoping someone takes over and fixes the tests for it.

[echuber2]

What needs editing is produce_headers.txt. To make sure this is correct, rather than just editing it in place to pass the tests, I think I need to also produce a mockup visually demonstrating the generated palettes before and after the proposed change. This should also illustrate the original flaw.

[hartwork]

Note: This is the raw values from ansi2html.style.SCHEME (not what get_styles(...) produces); pointed out by @echuber2 below.

render_scheme_as_html.py

[hartwork]

@echuber2 okay nice, let's drop it then and bring it back in an explicit saner version when (and if) we get bug reports about people missing the weird current behavior after the next release.

[hartwork]

FYI: The Terminal app built into Mac OS 11.6.1 comes with several profiles, with varying ANSI color palettes. Here is the palette for the "Basic" profile, which appears to be the default:

@echuber2 I extracted "Basic" from that^^ screenshot to be:

{
    "osx" (  # macOS theme "Basic"
        '#000000',
        '#800000',
        '#008000',
        '#808000',
        '#000080',
        '#800080',
        '#008080',
        '#808080',
        '#666666',
        '#e60000',
        '#00d900',
        '#e6e600',
        '#0000ff',
        '#e600e6',
        '#00e6e6',
        '#e6e6e6',
    )
}

Interestingly, our code on master has #c23621 as the second color, which I cannot find in any of the screenshots. To me that means, that commit 2176bc4 adding color theme "osx" had a different source of "os X terminal" or the scheme has change since 2013 on macOS side. @schettino72 could you help us on the origin on the colors for scheme "osx"? Can you confirm that default colors have changed since then?

@echuber2 I am currently considering to keep the spirit of the current osx palette unchanged but to put the previous effective light values in like this:

In [6]: from ansi2html.style import intensify, SCHEME

In [7]: new_osx = SCHEME["osx"][0:8] + tuple(
   ...:     intensify(color, dark_bg=True) for color in SCHEME["osx"][0:8]
   ...: )
   ...: new_osx
Out[7]: 
('#000000',
 '#c23621',
 '#25bc24',
 '#adad27',
 '#492ee1',
 '#d338d3',
 '#33bbc8',
 '#cbcccd',
 '#404040',
 '#ff7661',
 '#65fc64',
 '#eded67',
 '#896eff',
 '#ff78ff',
 '#73fbff',
 '#ffffff')

What do you think?

[echuber2]

I don't mind having more palettes available. In my most recent commit I had overwritten the osx one with the Basic palette from Jeff's screenshot and also added osx-brighter based on the "solid colors" palette. (Notably, the Basic palette seems to be very dark, and probably is meant for use on a white background.) Perhaps your new_osx should be put in under the name osx, and we can also have osx-basic and osx-brighter (or osx-solid-colors).

[hartwork]

Perhaps your new_osx should be put in under the name osx, and we can also have osx-basic and osx-brighter (or osx-solid-colors).

@echuber2 that sounds great, and osx, osx-basic and osx-solid-colors would be great naming (because of 1:1 mapping) and consistent with the existing dash in mint-terminal 👍

[echuber2]

Here's the updated preview. The three lines for each palette appear the same now, but the preview does not show the terminal background, etc. which still do change based on dark_bg.

[echuber2]

I think it's okay now but I'm not sure if something needs to be done about the codecov.

[ssbarnea]

I think it's okay now but I'm not sure if something needs to be done about the codecov.

Coverage looks ok to me. Sometimes we need to use common-sense and disregard what it reports.

[hartwork]

Hi @echuber2 .

I reviewed all but osx-solid-colors color values now. In general, I think the PR is in a great state now, very well done, thank you! 👍

My potentially final question here on details would be:

• Do you want me to double-check osx-solid-colors?
• Do we want to add function darken_bright_colors despite no tests and no users? I don't think it's a big problem, but I also find that the code is easy enough to re-do in 1 minute later if needed, and maybe just not adding that function is "even better"?
• There are "26 commits" in here according to GitHub. I would strongly advise against rebase-merging due to that. If everyone is expecting this to be squash-merged, then it's a non-issue. (Personally and elsewhere I would always ask to get PRs clean of merge commits and to rebase against latest master.)

Thanks for your work on this matter! 👍

[ssbarnea]

Hi @echuber2 .

I reviewed all but osx-solid-colors color values now. In general, I think the PR is in a great state now, very well done, thank you! 👍

My potentially final question here on details would be:

• Do you want me to double-check osx-solid-colors?
• Do we want to add function darken_bright_colors despite no tests and no users? I don't think it's a big problem, but I also find that the code is easy enough to re-do in 1 minute later if needed, and maybe just not adding that function is "even better"?
• There are "26 commits" in here according to GitHub. I would strongly advise against rebase-merging due to that. If everyone is expecting this to be squash-merged, then it's a non-issue. (Personally and elsewhere I would always ask to get PRs clean of merge commits and to rebase against latest master.)

Thanks for your work on this matter! 👍

If that is ok remove your requested changes status so I can squash-merge it without using admin rights. Or you can to the squash merge yourself.

In general I do only squash merged but sometimes I make mistakes (same reasons as you).

[hartwork]

@echuber2 thank you! 🙏