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.

Sign up for GitHub

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

Do not output colors e.g. into a pipe, unless forced. #1077

Merged
merged 2 commits into from
May 8, 2018
Merged

Do not output colors e.g. into a pipe, unless forced. #1077

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ba34e3e
Do not output colors e.g. into a pipe, unless forced.
lanurmi Apr 26, 2018
c04d441
Also support the CLICOLOR variable on Windows.
lanurmi May 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 30 additions & 1 deletion src/host/term_textColor.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,30 @@
static int s_currentColor = -1;
#endif

#if PLATFORM_WINDOWS
int canUseColors() {
return 1;
}
#else
int canUseColors() {
return isatty(1);
}
#endif

static const char *getenvOrFallback(const char *var, const char *fallback) {
const char *value = getenv(var);
if (value)
return value;
else
return fallback;
}

static int shouldUseColors() {
// CLICOLOR* documented at: http://bixense.com/clicolors/
return ((getenvOrFallback("CLICOLOR", "1")[0] != '0') && canUseColors())
|| getenvOrFallback("CLICOLOR_FORCE", "0")[0] != '0';
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like, given the code below, that this function could be called a LOT...
Like, it appears to do this work every time you wanna change colour, is that right?
Can't this just be executed once at startup?

Copy link
Contributor

@tvandijck tvandijck May 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I doubt it's of big impact... we don't set colors that often... only for errors, which are fatal generally, so it's a one time event...

I do agree that we could at startup however.. if @lanurmi has the time to do that, that would be great? but if not, I don't want to hold off for too long and let this PR sit around.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, give him a few days, and if not... just merge ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As long as premake doesn't color each character in regular output with a different color, is there a scenario where term_doSetTextColor() would be called a lot in the first place? What does one need to do wrong to get hundreds or thousands of colored lines (i.e. warnings or errors) currently?

I can make it cache the result, sure, but it adds some complexity, and we don't even know if shouldUseColors() is a hotspot in profiling.

Copy link
Member

Choose a reason for hiding this comment

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

is there a scenario where term_doSetTextColor() would be called a lot in the first place?

The unit tests now do this, sorry! It will be called 8200+ times when running the unit tests now, so it might be a good idea to cache the result. Additionally, Premake core might not call it a lot in regular usage, but that doesn't mean that modules can't call it a lot for any number of reasons.

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets merge it, and I'll take care of the caching...

}

int term_doGetTextColor()
{
#if PLATFORM_WINDOWS
Expand All @@ -25,12 +49,17 @@ int term_doGetTextColor()
void term_doSetTextColor(int color)
{
#if PLATFORM_WINDOWS
if (color >= 0)
if (color >= 0 && shouldUseColors())
{
SetConsoleTextAttribute(GetStdHandle(STD_OUTPUT_HANDLE), (WORD)color);
SetConsoleTextAttribute(GetStdHandle(STD_ERROR_HANDLE), (WORD)color);
}
#else

// Do not output colors e.g. into a pipe, unless forced.
if (!shouldUseColors())
return;

s_currentColor = color;

const char* colorTable[] =
Expand Down