search

ajalt / mordant

Multiplatform text styling for Kotlin command-line applications
https://ajalt.github.io/mordant/
Apache License 2.0
935 stars 33 forks source link

Accept more forced color values #156

Closed sschuberth closed 4 months ago

sschuberth commented 5 months ago

While at it, extract color values for TERM / COLORTERM to variables.

sschuberth commented 5 months ago

@ajalt, ./gradlew :extensions:mordant-coroutines:allTests also fails for me in master, so I guess the failure is not related to my changes?

ajalt commented 5 months ago

I'm -1 on this. FORCE_COLOR and COLORTERM are standard-ish variablers used by other libraries. I don't want really want mordant to behave differently than everyone else.

sschuberth commented 5 months ago

FORCE_COLOR and COLORTERM are standard-ish variablers used by other libraries. I don't want really want mordant to behave differently than everyone else.

I don't see a problem with being lenient WRT the accepted values. I mean, we're not setting these variables and expecting other applications to adhere to that.

Also, "24bits" (plural) is already accepted for COLORTERM although it's not specified e.g. at http://jdebp.uk/Softwares/nosh/guide/TerminalCapabilities.html.

Finally, this

https://github.com/ajalt/mordant/blob/13fce2b08872147598354c167bea1aefa5c799aa/mordant/src/commonMain/kotlin/com/github/ajalt/mordant/terminal/TerminalDetection.kt#L137-L139

says "We try to support them all", so I really believe this should be fine.

sschuberth commented 5 months ago

To give you more background, this is to make this work-around function better: Apparently, TERM does not support truecolor values, so we need to preferably look at COLORTERM to set FORCE_COLOR, but the latter should support all COLORTERM values then.

sschuberth commented 4 months ago

@ajalt, mind having another look? I've split the commit into two to make the intent more clear.

ajalt commented 4 months ago

So i looked at the workaround you posted and I'm a little confused: why are you copying COLORTERM into FORCE_COLOR rather than passing COLORTERM through?

sschuberth commented 4 months ago

why are you copying COLORTERM into FORCE_COLOR rather than passing COLORTERM through?

Not sure what you mean by "copying". Anyway, I've split into yet one more commit and hopefully clarified in the respective commits messages.

ajalt commented 4 months ago

Thanks for the additional clarification, but I'm still not in favor of this change. In the next major release, I plan to move in the opposite direction: I want to accept only the standard values of COLORTERM and FORCE_COLOR, rather than making up a bunch of new values and muddying the waters further.

sschuberth commented 4 months ago

Thanks for the additional clarification, but I'm still not in favor of this change.

I've split my commits so that it could still make sense to just pick the first commit of this PR. Feel free to do that.

I want to accept only the standard values of COLORTERM and FORCE_COLOR

Would you have a pointer to what the definite / standardized values of these are? To me, it do es not seem to clearly specified anywhere, which was my motivation to be lenient in the accepted values.