Thread: Re: BUG #16108: Colorization to the output of command-line hasunproperly behaviors at Windows platform

Hello everyone.

>  Please find attached a version that supports older Mingw versions and SDKs.

I have checked the patch source code and it seems to be working. But a
few moments I want to mention:

I think it is not good idea to mix the logic of detecting the fact of
TTY with enabling of the VT100 mode. Yeah, it seems to be correct for
current case but a little confusing.
Maybe is it better to detect terminal using *isatty* and later call
*enable_vt_mode*?

Also, it seems like if GetConsoleMode returns
ENABLE_VIRTUAL_TERMINAL_PROCESSING flag already set - we could skip
SetConsoleMode call (not a big deal of course).

Thanks,
Michail.

Hello.

> The patch about making color by default [1] introduces the function terminal_supports_color(), that I think is
relevantfor this issue. Please find attached a new version based on that idea.
 

I am not sure it is good idea to mix both patches because it adds some
confusion and makes it harder to merge each.
Maybe is it better to update current patch the way to reuse some
function later in [1]?

Also, regarding comment
> It is disabled by default, so it must be enabled to use color outpout.

It is not true for new terminal, for example. Maybe it is better to
rephrase it to something like: "Check if TV100 support if enabled and
attempt to enable if not".

[1] https://www.postgresql.org/message-id/flat/bbdcce43-bd2e-5599-641b-9b44b9e0add4@2ndquadrant.com

Hello.

Looks totally fine to me now.

So, I need to mark it as "ready to commiter", right?

On Mon, Feb 24, 2020 at 06:56:05PM +0100, Juan José Santamaría Flecha wrote:
> The patch was originaly reported for Windows, but looking into Peter's
> patch, I think this issue affects other systems unless we use stricter
> logic to detect a colorable terminal when using the "auto" option.
> Probably, the way to go is leaving this patch as WIN32 only and thinking
> about a future patch.

It is better to not mix issues.  You can actually bump on similar
coloring issues depending on your configuration, with OSX or even
Linux.

> The logic I have seen on new terminals is that VT100 is supported but
> disabled. Would you find clearer? "Attempt to enable VT100 sequence
> processing. If it is not possible consider it as unsupported."
>
> Please find attached a patch addressing these comments.

I was reading the thread for the first time, and got surprised first
with the argument about "always" which gives the possibility to print
incorrect characters even if the environment does not allow coloring.
However, after looking at logging.c, the answer is pretty clear what
always is about as it enforces colorization, so this patch looks
correct to me.

On top of that, and that's a separate issue, I have noticed that we
have exactly zero documentation about PG_COLORS (the plural flavor,
not the singular), but we have code for it in common/logging.c..

Anyway, committed down to 12, after tweaking a few things.
--
Michael