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

added color support for windows #102

Merged
merged 5 commits into from
Feb 9, 2015
Merged

added color support for windows #102

merged 5 commits into from
Feb 9, 2015

Conversation

Ryuuke
Copy link
Contributor

@Ryuuke Ryuuke commented Feb 7, 2015

No description provided.

@vitaut
Copy link
Contributor

vitaut commented Feb 8, 2015

Thanks a lot for the PR. A few comments/requests if you don't mind:

  • Could you leave the include of windows.h in format.cc and instead of conditionally defining the Color enum, add an array that maps standard ANSI colors to Windows-specific ones? Something like:

    // somewhere in format.cc:
    unsigned char COLORS[] = {
    0,
    FOREGROUND_RED | FOREGROUND_INTENSITY,
    FOREGROUND_GREEN | FOREGROUND_INTENSITY,
    ...
    };
    
  • GetStdHandle(STD_OUTPUT_HANDLE) can be called once in print_colored.

  • Could you please add checks if the calls to GetStdHandle and SetConsoleTextAttribute succeeded? Something like:

    HANDLE handle = GetStdHandle(STD_OUTPUT_HANDLE);
    if (handle == INVALID_HANDLE_VALUE)
      FMT_THROW(WindowsError(GetLastError(), "cannot get output handle"));
    
  • And finally, is there a way to unit test this as it is done for ANSI version in https://github.com/cppformat/cppformat/blob/master/test/format-test.cc#L1373?

@Ryuuke
Copy link
Contributor Author

Ryuuke commented Feb 8, 2015

Yeah no problem !

@vitaut
Copy link
Contributor

vitaut commented Feb 8, 2015

Thanks! This mostly looks good and my only concern now is the use of mutable global state (RESET_COLOR and reset_color_flag) which is not thread-safe. I suggest calling GetConsoleScreenBufferInfo to get the current color unconditionally and getting rid of these global variables.

@Ryuuke
Copy link
Contributor Author

Ryuuke commented Feb 8, 2015

As you like, In C++11 we could use std::call_once in this case for thread safety, right?

@vitaut
Copy link
Contributor

vitaut commented Feb 9, 2015

Yeah, but the library should be compatible with pre-C++11 compilers. Thanks updating the PR.

vitaut added a commit that referenced this pull request Feb 9, 2015
added color support for windows
@vitaut vitaut merged commit d5ab6ba into fmtlib:master Feb 9, 2015
@vitaut
Copy link
Contributor

vitaut commented Feb 9, 2015

I've merged it with minor changes (7004d1e & 9368b6a). However, the PrintColored test is failing: https://ci.appveyor.com/project/vitaut/cppformat/build/1.0.683.

@Ryuuke
Copy link
Contributor Author

Ryuuke commented Feb 9, 2015

The error in the test is coming from the codes below

  if(!GetConsoleScreenBufferInfo(handle, &infoCon))
    FMT_THROW(GetLastError(), "cannot get console informations");
 if(!SetConsoleTextAttribute(handle, color))
    FMT_THROW(GetLastError(), "cannot set console color");

it seems like it always throws with gtest, but I've tested in a project and it works...

@vitaut
Copy link
Contributor

vitaut commented Feb 9, 2015

Looks like the GetStdHandle doesn't work when you redirect the output from a console to a pipe with EXPECT_WRITE. Is there another way to test this, perhaps by accessing the console text directly via the Windows API?

@vitaut
Copy link
Contributor

vitaut commented Feb 19, 2015

I've moved the Windows implementation of print_colored to a separate branch, print-colored for now as it needs further testing. I'm happy to look into it myself, but it may take some time.

@vitaut vitaut mentioned this pull request Nov 19, 2015
@vitaut
Copy link
Contributor

vitaut commented Aug 11, 2019

@Ryuuke, could you please review the updated CONTIBUTING document, particularly the part about licensing, and let me know if you agree with it being applied to your contributions to {fmt}? The library is likely to be relicensed (#1073) so I'm collecting approval from all earlier contributors. Thanks!

@Ryuuke
Copy link
Contributor Author

Ryuuke commented Aug 19, 2019

Looks good to me !

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.

2 participants