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

Jump to bottom

Add log colours because they are pretty #237

Merged
merged 4 commits into from
Aug 23, 2022
Merged

Add log colours because they are pretty #237

merged 4 commits into from
Aug 23, 2022

Conversation

ASpoonPlaysGames
Copy link
Contributor

image

@GeckoEidechse
Copy link
Member

Soo pwetty 👀

I assume colour is only written to in console and we don't get any ANSI colour escape characters in logfile, right? ^^

@GeckoEidechse GeckoEidechse added needs testing Changes from the PR still need to be tested needs code review Changes from PR still need to be reviewed in code labels Aug 17, 2022
@ASpoonPlaysGames
Copy link
Contributor Author

I assume colour is only written to in console and we don't get any ANSI colour escape characters in logfile, right? ^^

image
Not 100% sure that this is the correct log, but all of the ones i have around the time i made the pr and uploaded that image are the same, no ANSI colour escape chars in sight as far as i can tell

@GeckoEidechse
Copy link
Member

So turns out colour printing doesn't seem to work on Windows 8.1 xD
It just prints in the usual white.

So I need someone on 10/11 to test to confirm this as working :,D

@GeckoEidechse GeckoEidechse added help wanted Extra attention is needed almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge and removed needs code review Changes from PR still need to be reviewed in code labels Aug 18, 2022
@ASpoonPlaysGames
Copy link
Contributor Author

screenshot from discord
image

Alystrasz
Alystrasz approved these changes Aug 19, 2022
View reviewed changes
Copy link
Contributor

@Alystrasz Alystrasz left a comment

Choose a reason for hiding this comment

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

Tested on Windows 10.
LGTM

GeckoEidechse
GeckoEidechse approved these changes Aug 19, 2022
View reviewed changes
Copy link
Member

@GeckoEidechse GeckoEidechse left a comment

Choose a reason for hiding this comment

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

Colours confirmed working when testing on Windows 8.1 :3

@GeckoEidechse GeckoEidechse removed help wanted Extra attention is needed needs testing Changes from the PR still need to be tested labels Aug 19, 2022
@GeckoEidechse
Copy link
Member

GeckoEidechse commented Aug 19, 2022 •
edited
Loading

Also confirmed again, no ANSI colour escape codes or similar in text logs ^^

@GeckoEidechse GeckoEidechse removed the almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge label Aug 23, 2022
@GeckoEidechse GeckoEidechse merged commit 812893d into R2Northstar:main Aug 23, 2022
GeckoEidechse pushed a commit that referenced this pull request Aug 25, 2022
@ASpoonPlaysGames @GeckoEidechse
Add log colours because they are pretty (#237)
a97f1f0 
* log colours :)

* commend the freopen stuff (it causes problems)

* formatting

* add explanatory comment
@Alystrasz Alystrasz mentioned this pull request Sep 10, 2022
Closed
GeckoEidechse added a commit that referenced this pull request Sep 13, 2022
@GeckoEidechse
Revert "Add log colours because they are pretty (#237)"
57d2110 
This reverts commit a97f1f0.
GeckoEidechse added a commit that referenced this pull request Sep 13, 2022
@GeckoEidechse
Revert "Add log colours because they are pretty (#237)"
4625e5e 
This reverts commit a97f1f0.
GeckoEidechse added a commit that referenced this pull request Sep 13, 2022
@GeckoEidechse
Revert "Add log colours because they are pretty (#237)"
ec46514 
This reverts commit a97f1f0.
@GeckoEidechse
Copy link
Member

Reverted in v1.9.6 due to causing some issues with client freezes, mostly related to Viper. The exact cause is still unknown.

This was referenced Sep 13, 2022
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GeckoEidechse GeckoEidechse GeckoEidechse approved these changes

@Alystrasz Alystrasz Alystrasz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Northstar Roadmap
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ASpoonPlaysGames @GeckoEidechse @Alystrasz