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

[Windows, 4.0] Detect new Windows Terminal and disable unsupported set_console_visible code. #55966

Merged
merged 1 commit into from
Dec 16, 2021

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Dec 15, 2021

Tested on Windows 11 21H1(22000.348) with:

  • Windows Console Host (Old terminal)
  • ConEmu 210912
  • Windows Terminal 1.11.2921.0

Bugsquad edit: Closes #55971.

@bruvzg bruvzg added this to the 4.0 milestone Dec 15, 2021
@bruvzg bruvzg requested a review from a team as a code owner December 15, 2021 18:05
@bruvzg bruvzg changed the title [Windows] Detect new Windows Terminal and disable unsupported set_console_visible code. [Windows, 4.0] Detect new Windows Terminal and disable unsupported set_console_visible code. Dec 15, 2021
HANDLE hStdOut = GetStdHandle(STD_OUTPUT_HANDLE);
DWORD dwMode = 0;
if (GetConsoleMode(hStdOut, &dwMode)) {
return ((dwMode & ENABLE_VIRTUAL_TERMINAL_PROCESSING) == ENABLE_VIRTUAL_TERMINAL_PROCESSING);
Copy link
Member

@mhilbrunner mhilbrunner Dec 16, 2021

Choose a reason for hiding this comment

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

So if I understand it right, this is mostly a heuristic: ENABLE_VIRTUAL_TERMINAL_PROCESSING was added for the Windows Terminal in the Windows 10 Anniversary update, and isn't enabled by default in the legacy console, but is enabled by default in Windows Terminal.

Thus we can check if it is enabled and assume we're likely running in a Windows Terminal. In theory, we could also be running in another console that has this turned on manually.

This looks acceptable to me, but maybe we could have a comment explaining/documenting this? Maybe we also want to link to the original Github issue for future readers. (#54076)

That would

  1. explain this/make it more readable
  2. ensure we remember to remove it if this is fixed/not revelant anymore upstream

Copy link
Member

Choose a reason for hiding this comment

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

Adding a short comment sounds good yeah. The issue can be referenced as GH-54076 in text, that's how I do it usually (it's easy enough to get to the URL from there, without having to hardcode all kinds of github.com all over the place).

Copy link
Member

Choose a reason for hiding this comment

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

That being said, we might prefer to wait for the approach taken in #55987 and thus only merge this workaround for 3.4?

Copy link
Member

Choose a reason for hiding this comment

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

I'll merge as is for now as I want to cherry-pick #55967 for 3.4.1, and it's good to keep things consistent. Comments can be added later, or won't be needed if we go with the approach in #55987 and thus undo this workaround.

@akien-mga akien-mga merged commit 33e0338 into godotengine:master Dec 16, 2021
@akien-mga
Copy link
Member

Thanks!

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.

Game Output/Log goes to new terminal window on start and closes on game end
3 participants