Skip to content

Conversation

@AlexeiScherbakov
Copy link

Basic functionality, not integrated it Colorful.Console class
see - #24

@tomakita tomakita self-assigned this Jun 21, 2017
@Alexejhero
Copy link

This pull request has not been updated since 2017. Is this still being worked on?

@AlexeiScherbakov
Copy link
Author

AlexeiScherbakov commented Jun 21, 2019 via email

@Alexejhero

This comment has been minimized.

@AlexeiScherbakov
Copy link
Author

AlexeiScherbakov commented Jun 21, 2019 via email

@tomakita
Copy link
Owner

tomakita commented Jun 21, 2019

Hi @AlexeiScherbakov ,

You're right, work will need to be done to add .NET Core support. I don't have to time begin working on this yet, but it's on my list of things to do, as .NET Core 3.0 is almost upon us. I'll also accept PRs for .NET Core support :)

Hi @AlexejheroYTB ,

Thanks for the reminder. I never merged this PR because I didn't have access to Windows 10, and I was worried that I wouldn't be able to test the PR version of the code. I have Windows 10 now, and will test this out over the weekend, so that @AlexeiScherbakov 's PR doesn't go to waste.

@tomakita
Copy link
Owner

tomakita commented Jun 22, 2019

@AlexeiScherbakov

I've looked through and tested the code. It looks good, and works very well. Thanks for the PR!

I have a thought about how we can detect VT support, though: I'm thinking that, rather than using operating system detection, we should just try to detect whether or not the feature is available. This might work better if e.g. VT support is removed in later versions of Windows. Your code already does this -- I'll add a bit more code once you merge this PR, but your code does most of what we'll need to do. So if we go this route of feature detection instead of operating system detection, I think we can remove the OperationSystemDetector class. We could also remove lines 23-34 (inclusive) from NativeMethods.cs. What do you think?

BTW, you don't have to make these changes, I can make them after I merge the PR.

}

}
else if (File.Exists("/proc/sys/kernel/ostype"))
Copy link

Choose a reason for hiding this comment

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

wtf?
Please, use RuntimeInformation.IsOSPlatform()

@Eregrith
Copy link

Eregrith commented Jan 8, 2020

@tomakita Are you planning to expose a capability-test function to clients of the lib? It would be useless to allow 256 colors but not tell the dev if more than 16 colors can be safely used in the style config because it would render very ugly when not 256-capable and configured colors start to overflow the 16 buffer :/

@tomakita
Copy link
Owner

tomakita commented Jan 9, 2020

@Eregrith That's the plan, though it hasn't yet been decided exactly how that will be implemented. See #29 (comment) for more info.

Btw, I'm very much open to PRs in this area (perhaps one that is based upon Alexei's good work in this PR). I haven't merged this one because there's more work to be done in order to make it prod ready, and I haven't had the time to do it, yet.

@NotoriousRebel
Copy link

any updates?

@dotnetshadow
Copy link

Any chance to get this merged in?

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.

7 participants