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

Tweak console error/warning print colors #98796

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

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Nov 3, 2024


  • Colorize the "at:" line in the same color as the error/warning, but in a more faint color.
  • Standardize the console coloring code across platforms (since ANSI escape codes can now be used on Windows).

Preview

VS Code terminal

Dark theme

Before  After
VS Code dark before VS Code dark after

Light theme

Before  After
VS Code light before VS Code light after

kitty

Before  After
kitty before kitty after

Keywords for easier searching: color, terminal, commandline, command line, CLI

- Colorize the "at:" line in the same color as the error/warning,
  but in a more faint color.
- Standardize the console coloring code across platforms
  (since ANSI escape codes can now be used on Windows).
@Calinou Calinou added this to the 4.4 milestone Nov 3, 2024
@Calinou Calinou requested review from a team as code owners November 3, 2024 13:47
// This prevents Godot from writing ANSI escape codes when redirecting
// stdout and stderr to a file.
//
// FIXME: Is there a way to check whether stdout is a TTY on Windows?
Copy link
Member

Choose a reason for hiding this comment

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

@hsvfan-jan
Copy link

In general, I really like this but I wonder if there is a better colour for the dark theme. Right now the readability is reduced for errors since the value contrast has become lower, see my black and white version. Warnings pretty much perfectly retain readability
Screenshot_2024-11-03_at_14 52 05

@Calinou
Copy link
Member Author

Calinou commented Nov 3, 2024

In general, I really like this but I wonder if there is a better colour for the dark theme.

Unfortunately, we don't have control over this if we want a color that looks good on both dark and light themes. Using 256-color or 24-bit colors for this kind of stuff can be a double-edged sword, as you can no longer have a different color between a dark and light theme. With the basic 16 ANSI colors, their exact colors are determined by the terminal theme. This means the theme author can choose colors that look good given the background color (or at least try to).

Most of the time, the trace location isn't that important for users either1, so it's not necessarily a bad thing that it becomes less prominent. The source file path can still be Ctrl + clicked for developers who are running Godot from a source repository.

Footnotes

  1. It only points to the engine's C++ code, not to the user's scripts.

// stdout and stderr to a file.
//
// FIXME: Is there a way to check whether stdout is a TTY on Windows?
const char *red = "\E[0;91m";
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably avoid using \E as our escape code, as it doesn't actually appear in the standard1. All existing instances can be converted to \x1b or \033 instead, similar to what we use in SCons

Suggested change
const char *red = "\E[0;91m";
const char *red = "\x1b[0;91m";

Footnotes

  1. https://en.wikipedia.org/wiki/Escape_sequences_in_C#endnote_Note1

@JyveAFK
Copy link

JyveAFK commented Nov 4, 2024

Can we have a few peeps colour blind check this? As it seems (to my colourblind eyes) really make it hard to read, it's too faint against the dark background.

@bruvzg
Copy link
Member

bruvzg commented Nov 4, 2024

Dark themes look fine to me, but red in light themes is barely readable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants