Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

options: Add truecolor to control the mode #2867

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JoeKar
Copy link
Collaborator

@JoeKar JoeKar commented Jul 15, 2023

Currently even 256 color schemes like the default one don't look as defined and then most probably expected due to used terminal palette. So I suggest to determine the true color support based on the already mentioned $COLORTERM environment variable and furthermore the overall control should be able with a micro internal option to prevent the additional definition of a further user defined environment variable.

w/o vs w/ true color support/mode:
image

This could close #2859 too.


Note: The change will take effect after the next start of `micro`.

default value: `off`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not make auto the default?

And if make it the default, then also need to let the user override auto by setting MICRO_TRUECOLOR=1, for cases when the terminal supports truecolor but does not set COLORTERM (there are such terminals out there). But looks like your code is already doing that, although the above description does not mention it, it only mentions that MICRO_TRUECOLOR=1 overrides off.

BTW also maybe it's better to let MICRO_TRUECOLOR override auto only, not let it override off, i.e. maybe truecolor=off should have higher priority than MICRO_TRUECOLOR=1? (Or perhaps it's not so important which of the two has higher priority...)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also need to let the user override auto by setting MICRO_TRUECOLOR=1

On second thought, the user could as well override it by setting truecolor to on. And generally, looks like the MICRO_TRUECOLOR env var becomes superfluous once we have the truecolor option. But if we do need to keep supporting MICRO_TRUECOLOR for backward compatibility, then we should keep its behavior consistent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not make auto the default?

This would directly mean changing the "expected"/known behavior, because it's documented till now with...

"True color support in micro is off by default [...]"

Furthermore it's common to have the user-defined terminal color palette applied to the editor/application. So from that perspective I thought about this as well, but came to the conclusion that the change would be to drastic.

And generally, looks like the MICRO_TRUECOLOR env var becomes superfluous once we have the truecolor option.

Yes, definitely. With the introduction of the options the control via additional env vars should have been removed to have a single point of truth. If @zyedidia (as project owner) endorses the removal of the env var, then I'm fully for the removal of it too and set the option default to auto or off.
The only strange setting would be to have $MICRO_TRUECOLOR=0and truecolor=auto|on. From my perspective the activation will override the disabled env var...so I documented this now explicitly.

Copy link
Collaborator

@dmaluka dmaluka Sep 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would directly mean changing the "expected"/known behavior, because it's documented till now

Yes, but it doesn't necessarily mean that it is the most desired behavior. And weren't you the one advocating for making autodetection work out of the box (in #2859 (comment))?

In particular, regarding:

Furthermore it's common to have the user-defined terminal color palette applied to the editor/application.

It is common for 16-color palettes, but I'm not sure how common it is for 256-color palettes. And anyway, I'd assume that if a user wants to use his custom terminal colors in micro, he'd rather prefer using a 16-color or 256-color colorscheme accordingly, not a truecolor colorscheme (which probably wasn't designed to look as good in custom colors as in true colors), so that truecolor settings wouldn't affect him.

Anyway... as I've discovered that the tcell library actually enables truecolor by default if available (as I've noted in my other comment above), and micro deliberately disables it by default by setting TCELL_TRUECOLOR=disable, looks like it is actually an intended behavior. But I don't quite understand what is the reason for doing so.

@zyedidia could you please clarify:

  1. What was the reason for overriding tcell's default behavior (enable truecolor, if available) with micro's behavior: disable truecolor, unless MICRO_TRUECOLOR=1 is set?
  2. Some of the colorschemes that are mentioned in help colors as 256-color schemes (including the default colorscheme) are actually specified in their colorscheme files with RGB values, not with 256-color codes (and as a result, they are affected by truecolor being enabled or not). What was the reason for that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but it doesn't necessarily mean that it is the most desired behavior.

Yep, I'd like to have it on by default too, but it isn't my decision and the only intention was to not change the default behavior.

And weren't you the one advocating for making autodetection work out of the box (in #2859 (comment))?

Yes, I was and I still be. As you found out it's already done by tcell and this is fine.

Copy link
Contributor

@Neko-Box-Coder Neko-Box-Coder Jun 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only strange setting would be to have $MICRO_TRUECOLOR=0 and truecolor=auto|on. From my perspective the activation will override the disabled env var...so I documented this now explicitly.

@JoeKar

I think we should respect $MICRO_TRUECOLOR first and truecolor second, so that this allows us to make truecolor to be auto|on by default.

Not the other way around where we default truecolor to off and have to figure out a way to make it to on by default in the future.

Then we can mark $MICRO_TRUECOLOR to be legacy / deprecated and transition to remove it at some point.

Any chance that we can merge this PR at some point?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dmytro summarized it really good with his #2867 (comment).
The confusing aspect is the different default behaviors between tcell and micro. In general it's off by design of micro while using a default color scheme which has a different look between the modes.
Changing the default behavior again means breaking with compatibility/expectation.

Copy link
Contributor

@Neko-Box-Coder Neko-Box-Coder Jun 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JoeKar
Assuming we respect $MICRO_TRUECOLOR first but have truecolor to be auto|on

The only group of people I can think of that will be affected by this change is

  • People who don't have $MICRO_TRUECOLOR set to 1 and use a truecolor color scheme

which is a wrong behavior and they are probably not getting what they think they are getting anyway.

But unfortunately, this also affects the default color scheme. We can either:

  • Convert it to 16 palette which would affect the people that is using it as true color
  • Leave it as it is which would affect the people that is using it as (16 or ) 256 palette color

I am leaning toward the second option but either way seem fine to me,

Right, and we also need to correct the color schemes we have to match the title (or the other way around, idk)
Assuming color schemes with tc suffix are true color and ones with 16 suffix are 16 palette and ones with no suffix are 256 palette.

Here are all the ones that the name do not match the color depth:

Changing the name would be the easiest, but would also break people's config. We can probably do something like search for tc and 16 suffix if the one in config can't be found, and then use that (and also give a warning to tell the user to correct the color scheme name)

I think either way this is going to be breaking compatibility anyway at some point (unless we never switch to truecolor as default), we might as well do it now.

@JoeKar JoeKar force-pushed the feature/truecolor-support-detection branch from ac533d8 to e6aedd0 Compare September 11, 2023 20:26
// Is true color support available?
truecolorAvailable := (os.Getenv("COLORTERM") == "truecolor") ||
(os.Getenv("COLORTERM") == "24bit") ||
(os.Getenv("COLORTERM") == "24-bit")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've found that this detection is actually already done by tcell: https://github.com/gdamore/tcell/blob/08c7757cd1c98810e46975da457c033dc8657ad8/terminfo/terminfo.go#L688
See also its readme: https://github.com/gdamore/tcell/blob/08c7757cd1c98810e46975da457c033dc8657ad8/README.md#24-bit-color

So no need to do it again in micro itself. All micro needs to do (and what it already does, a few lines below) is to set or not to set TCELL_TRUECOLOR=disable.

Also it means that in your implementation there is no difference between auto and on, since on does not really force truecolor, it only enables tcell's truecolor detection by not setting TCELL_TRUECOLOR=disable.

IOW, it turns out that your PR does not provide new functionality. The automation is already there, only that it is disabled by default (but your PR does not enable it by default either). The user can enable truecolor autodetection by setting MICRO_TRUECOLOR=1, and if tcell fails to detect truecolor, the user can force it by setting COLORTERM=truecolor.

Although your PR adds possibility to use an option instead of env vars for that, but I'm not sure if that brings a real improvement in user experience, e.g. considering that it still takes no effect until the next start of micro.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So no need to do it again in micro itself.

Yes, due to the fact that it's already done within tcell there is no need to do it twice. I fully confirm. Most probably I should have checked tcell earlier too by following the flag...
My fault.

Although your PR adds possibility to use an option instead of env vars for that, [...]

Hm, ok...then it's most probably no longer needed at all. Then I would change my mind to better change the default of the MICRO_TRUECOLOR (use it to explicitly disable TC) handling, since I don't like the env var approach to enable that feature.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upon more thought, maybe actually it would be nice to have the truecolor option (replacing the MICRO_TRUECOLOR variable) as a single interface point for controlling the truecolor behavior, which would merely do the following: off value sets TCELL_TRUECOLOR=disable to disable truecolor, on sets COLORTERM=truecolor to force truecolor, auto does nothing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, but didn't remove MICRO_TRUECOLOR yet.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we introduce the new method while keep the old method, IMO things become confusing: need to specify what overrides what, and so on.

Let's try not to introduce more mess.

Let's first figure out what exactly do we want. Starting from the question do we want autodetection enabled by default or not, and why.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will vote for the activation of the auto detection by default, because it reduces the necessary configuration and the trouble by switching between different types of color/highlighting schemes.
Furthermore the configuration interface via MICRO_TRUECOLOR should be declared deprecated to directly removed, to return to the single point of configuration.


Note: The change will take effect after the next start of `micro`.

default value: `off`
Copy link
Collaborator

@dmaluka dmaluka Sep 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would directly mean changing the "expected"/known behavior, because it's documented till now

Yes, but it doesn't necessarily mean that it is the most desired behavior. And weren't you the one advocating for making autodetection work out of the box (in #2859 (comment))?

In particular, regarding:

Furthermore it's common to have the user-defined terminal color palette applied to the editor/application.

It is common for 16-color palettes, but I'm not sure how common it is for 256-color palettes. And anyway, I'd assume that if a user wants to use his custom terminal colors in micro, he'd rather prefer using a 16-color or 256-color colorscheme accordingly, not a truecolor colorscheme (which probably wasn't designed to look as good in custom colors as in true colors), so that truecolor settings wouldn't affect him.

Anyway... as I've discovered that the tcell library actually enables truecolor by default if available (as I've noted in my other comment above), and micro deliberately disables it by default by setting TCELL_TRUECOLOR=disable, looks like it is actually an intended behavior. But I don't quite understand what is the reason for doing so.

@zyedidia could you please clarify:

  1. What was the reason for overriding tcell's default behavior (enable truecolor, if available) with micro's behavior: disable truecolor, unless MICRO_TRUECOLOR=1 is set?
  2. Some of the colorschemes that are mentioned in help colors as 256-color schemes (including the default colorscheme) are actually specified in their colorscheme files with RGB values, not with 256-color codes (and as a result, they are affected by truecolor being enabled or not). What was the reason for that?

@JoeKar JoeKar force-pushed the feature/truecolor-support-detection branch from e6aedd0 to c462bc2 Compare September 15, 2023 20:41
@JoeKar JoeKar force-pushed the feature/truecolor-support-detection branch from c462bc2 to aa34182 Compare October 26, 2023 18:53
- `auto`: leaves the detection to `tcell`
- `on`: forced usage, same as `$MICRO_TRUECOLOR` set to 1
- `off`: default, can be overriden by `$MICRO_TRUECOLOR`
@JoeKar JoeKar force-pushed the feature/truecolor-support-detection branch from aa34182 to d06cf54 Compare March 15, 2024 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Colours Don't Match to Set Colours
3 participants