suggested color changes for cherry-picker output

[smontanaro]

Per #116, here's a first cut at dimming the output of git cherry-pick and making the output of cherry_picker more prominent.

[hugovk]

Please could you share some screenshots of output?

We should probably support https://no-color.org/ so people who have difficulty reading coloured output can disable it by setting a NO_COLOR environment variable.

Unfortunately Click doesn't support it, they suggest project implement it themselves:

https://github.com/pallets/click/issues/1090, https://github.com/pallets/click/issues/1498, https://github.com/pallets/click/issues/2207, https://github.com/pallets/click/issues/2282

However, https://no-color.org/ does list https://github.com/kdeldycke/click-extra, perhaps we can use that?

[smontanaro]

Please could you share some screenshots of output?

Sure. Here's the output of that little clickex script I attached earlier. (I had to give it a "txt" extension to get it by the GitHub censor.) Note at this case that I haven't had occasion to use this yet, and my failures to properly use it previously mean I have no complete run to snapshot.

Thanks for the no-color.org and click-extra pointers. I will check them out.

[smontanaro]

@hugovk I pushed changes to support NO_COLOR and --color/--no-color. I imagine it would have been simpler if I used click-extra, but I wasn't sure how that might affect things. I haven't used cherry-picker much, and click even less. This was easier for my simple brain to wrap itself around. pytest runs with no errors, though at this point I've not attempted to add any further unit tests.

I have it installed locally and will use it when I can. (I guess I should make some more CPython doc changes...) You can check out this and try it locally as well.

[hugovk]

I also installed it locally and finally got a conflict (macOS Sonoma 14.2.1/iTerm2 3.4.23/starship 1.13.1):

Here's how the clickex example looks:

[smontanaro]

Thanks Hugo. No output from after executing cherry_picker --continue?

click.style supports only a limited number of colors by names (red, black, etc). I'm certainly no color whiz and realize some people who are red/green color blind might not distinguish the red text very well. My main concern was to push git cherrypick output into the background and highlight cherry_pciker output. I'm open to alternate suggestions for "bright" and "dim."

Also, someone more familiar with click-extra might want to replace my simplistic color support and see if that makes more sense. If any documentation changes I've worked on recently ever make it through the mill and generate backport conflicts I might try that, but as a first cut, the bright/dim seems okay to me.

[hugovk]

No output from after executing cherry_picker --continue?

I didn't actually do that step, I let the original PR author resolve it :) But trying again I get:

[hugovk]

click.style supports only a limited number of colors by names (red, black, etc). I'm certainly no color whiz and realize some people who are red/green color blind might not distinguish the red text very well. My main concern was to push git cherrypick output into the background and highlight cherry_pciker output. I'm open to alternate suggestions for "bright" and "dim."

Here's some screenshots of simulating colour blindness with the https://github.com/oftheheadland/Colorblindly browser extension. Quite a few of them show the second "red" chunk as less obvious than the first "white" chunk:

Details

       

[smontanaro]

Thanks. I wasn't aware of that browser extension and installed it. Viewing my original screenshot (white background) using the various settings, the cherry_picker output is always more prominent than the git cherry-pick output. I suppose if click can automatically detect the background color it can make better foreground color choices. Failing that, perhaps a --background color option could be added to either set the background color or tell click what it is, so more suitable foreground color choices can be made. In addition, --bright and --dim options could be added to override the default choices.

I'll see about adding --background, --bright and --dim options first. If that's not sufficient, we can discuss other paths. Again, maybe click-extra would obviate all of this.

[hugovk]

It would be good to figure out some good defaults, as I'm guessing most won't want to fine tune the options.

The colour names supported by Click:

https://click.palletsprojects.com/en/8.1.x/api/#click.style

[encukou]

The 16 named colours are determined by the terminal. If there's e.g. not enough contrast between background and red in the default settings, you should probably report it to the terminal devs.

That also means you shouldn't use direct RGB values, or the 256-colour palette. (Unless you also paint the background using RGB, which looks bad if it's not a “full-screen” app.)

So, my suggestion: stick to 16 named colours, implement NO_COLOR & --color/--no-color to opt out, but don't over-analyze how your terminal renders things.

[smontanaro]

@encukou Shouldn't the user have the choice to specify colors of their choosing? After all, nobody will know their preferred color palette better. (In my experience, some people have pretty unusual color preferences.) In my sandbox, in addition to the named colors, I currently support hex strings (six-digit and three-digit) which I convert to the relevant tuple of ints. If something doesn't work right, the user is welcome to choose different colors.

Also, it seems with my white background on my Mac and @hugovk's black backrgound on his system, it's pretty obvious finding a single "bright" and "dim" color pair that will work just with those two backgrounds will be hard. And what of users who like grey backgrounds?

[smontanaro]

New version. Colors in the form 0xRRGGBB, 0xRGB or (red, green, blue) are supported. If none of those match, the input color is simply assumed to be a named color supported by Click. (I could verify that I suppose, though I don't see a canonical list in click itself, just a list in the click.style docstring.)

[smontanaro]

Any further feedback? Whether or not they are generally useful, I do have defaults for the bright and dim colors:

BRIGHT = "red"
DIM = "0x808080"

Those work well for me (white background, not red/green color blind). I'm more than happy to pick a different bright color if there is one which people feel would be generally useful in most cases (various types of colorblindness, common terminal background colors).

[hugovk]

See the CI failures: we can't use the match statement (new in 3.10) because we support Python 3.8+.

I'm also not sure if we need to expose --bright and --dim on the CLI.

[smontanaro]

Sorry, I wasn't aware of the 3.8 support. I've removed the match statement and executed tox (which I'd been ignoring before). I don't have a 3.8 interpreter, but it ran fine back to 3.9:

...
py39: OK ✔ in 15.81 seconds
py38: skipped because could not find python interpreter with spec(s): py38
.pkg: _exit> python /Users/skip/miniconda3/envs/python311a/lib/python3.11/site-packages/pyproject_api/_backend.py True hatchling.build
  py312: OK (22.74=setup[11.56]+cmd[11.18] seconds)
  py311: OK (17.61=setup[6.50]+cmd[11.11] seconds)
  py310: OK (16.50=setup[5.93]+cmd[10.57] seconds)
  py39: OK (15.81=setup[5.75]+cmd[10.06] seconds)
  py38: SKIP (0.44 seconds)
  congratulations :) (73.14 seconds)

You mentioned not needing --bright or --dim on the command line. How else would users set those colors? I see mention of a .cherry_picker.toml file, and an example in the readme. Would something like this work?

color = true
bright = "blue"
dim = "0x999944"

If so, how would I tell click to look there for those option values? (I've really never used click before, and was just parroting what I saw in the existing code.)

[encukou]

Shouldn't the user have the choice to specify colors of their choosing? How else would users set those colors?

I do not want to set the colors for each small tool I use. IMO, it's more important to pick good defaults than to allow customization.

As mentioned before, 808080 is not a good default, if you don't also set a specific background color.

[smontanaro]

@hugovk I think I figured out the argument/option thing and made bright and dim arguments. Tests pass, including tox run down to 3.9. Let me know if you agree with the latest changes.

[smontanaro]

Shouldn't the user have the choice to specify colors of their choosing? How else would users set those colors?

I do not want to set the colors for each small tool I use. IMO, it's more important to pick good defaults than to allow customization.

As mentioned before, 808080 is not a good default, if you don't also set a specific background color.

We seem to be at an impasse. In my mind, the user sets a desired terminal background color. The cherry-picker user can set a text foreground color. I do not ever set a background color for the text. In this particular application, it would likely have the opposite of the desired effect, making the git cherry-pick output fade into the background (but still be readable).

I don't believe there is a perfect "dim" color given that users are free to choose whatever tickles their fancy as their preferred terminal background. You like black. I like white. Tell me a good "dim" foreground color you think would work for both.

If using color to distinguish cherry-picker output from git output isn't going to work, think of some other way to make cherry-picker's output more obviously what the user should be paying most attention to.

[encukou]

I want cherry-picker to “just work”. If its output becomes unreadable with the default configuration, and I need to study how to configure it, that's a regression for me.

Tell me a good "dim" foreground color you think would work for both.

cyan, for example. Or another one of the named colours provided by the terminal, which should work with its default background. Or keep this in the default colour and only highlight the important parts (using named colours).