search

gui-cs / Terminal.Gui

Cross Platform Terminal UI toolkit for .NET
MIT License
9.54k stars 681 forks source link

Colors: Update Color System (TrueColor, Blink, Underline, etc...) for v2 (Master Issue) #457

Open tig opened 4 years ago

tig commented 4 years ago

This is the master Issue for revamping the Color system in v2. Dependencies:

See also:

Todos

tig commented 1 year ago

The fix for #666, PR #2612 is providing groundwork for this. Anyone choosing to start on this PR should base it on that PR if you do it before merged in to v2_develop.

adstep commented 1 year ago

Hey @tig, is #2612 still the right branch to start from if I'm playing around with porting over the changes from #1628?

tznind commented 1 year ago

Hey @tig, is #2612 still the right branch to start from if I'm playing around with porting over the changes from #1628?

That's correct, it is the most recent console driver refactoring branch. Please can you also consider taking/adapting this https://github.com/veeman/gui.cs/pull/2 (tznind:true-color) PR which I opened into https://github.com/gui-cs/Terminal.Gui/pull/1628 it adds the following:

I would really love true color support for so many cool things. Especially for colored icons in FileDialog, gradient borders etc. Let me know if you want to collaborate on this.

adstep commented 1 year ago

Sure I can give it a try this week. Seems straightforward enough given the pre-existing examples.

Is this the right place to loop back if I have any troubles?

tznind commented 1 year ago

Sure I can give it a try this week. Seems straightforward enough given the pre-existing examples.

Awesome, this is really exciting!

Is this the right place to loop back if I have any troubles?

Sure, post any initial/logistics issues here. When there is a PR (even a draft one) then we can talk about more specifics there.

I guess since #2612 is +10,135/−9,977 vs v2_develop it makes most sense to branch off tig:v2_fixes_666_console_driver_dupes and probably target tigs branch with a draft PR with the updated true color code. Otherwise the diff will be lost in all the other changes (a lot are just layout) and hard to review.

We can always retarget once tig:v2_fixes_666_console_driver_dupes is merged if @tig wants to merge them in seperately.

tig commented 1 year ago

It will likely be a week or so before I can spend time on my console dupe PR, so I concur you should party on it and submit PRs to it for the color stuff.

Really excited to see someone picking this up! Thank you!

adstep commented 1 year ago

Got it running. See https://github.com/tig/Terminal.Gui/pull/15.

BDisp commented 1 year ago

Got it running. See tig#15.

This is a great work. Thanks for providing this. Does it also works on another drivers (CursesDriver and NetDriver)?

tznind commented 1 year ago

Its ok if it only works for console driver to start with.

We can always add support in the other drivers after. As long as the fallback mechanism is working and the true color class design is sound.

The original work was done by @veeman so it might be we don't have the expertise between us to do the other drivers yet.

adstep commented 1 year ago

Does it also works on another drivers (CursesDriver and NetDriver)?

The PR doesn't add support for CursesDriver or NetDriver.

The PR just enables WindowsDriver to use VT sequences to describe the requested console color. This should work consistently because there's a clear mechanism to detect whether TrueColor support is possible by checking the version of Windows.

AFAIK Curses uses a mechanism where, if you aren't using one of the 16 default Console colors, you have to register the color you want to use and get returned an identifier to pass along for future calls. This has some limitation where you can only have a couple thousand cached colors due to some restrictions of Curses. If you want to use VT sequences, when using Curses, you'd need to figure out some other means of detecting whether they'd be supported.

tznind commented 1 year ago

If you want to use VT sequences, when using Curses, you'd need to figure out some other means of detecting whether they'd be supported.

I think this should be handled with config files in the same way we support Nerd Fonts (#2613).

So any end user could add to their .tui folder config:

"VTTrueColor.Enable": true

Or if the vast majority of modern consoles support this we could default it to true and let end users opt out with a setting of false in config.

BDisp commented 1 year ago

@adstep I don't know if you already seen my explanation why some colors has the value -1, in https://github.com/tig/Terminal.Gui/pull/15#discussion_r1243574508.

BDisp commented 1 year ago

Great stuff here: https://gist.github.com/fnky/458719343aabd01cfb17a3a4f7296797?permalink_comment_id=4619910#gistcomment-4619910

tznind commented 1 year ago

@BDisp is it still possible to output specific escape sequences in CursesDriver?

I know @tig mentioned reducing/eliminating reliance on ncurses methods in https://github.com/gui-cs/Terminal.Gui/pull/2612#issuecomment-1565264597 in favour of direct use of escape codes (he references https://github.com/gui-cs/Terminal.Gui/discussions/1107)

I found and tested some of the console escape codes in the following video on my linux box and found they work. Also sounds like most modern linux terminals are going to support at least 256 if not true color escapes: https://www.youtube.com/watch?v=RKPsd4tD9dQ

See this test script that shows how to output escape sequences for 256 colors https://github.com/BrodieRobertson/scripts/blob/ae0077d38777151e07824410fade622eb93f0ca0/color256#L4

If the escape codes approach taken in https://github.com/tig/Terminal.Gui/pull/15 would still work in linux then I think we should just be practical about things. Specifically:

Sorry that I am a little late to the party on this as I haven't gotten so deep into all the various schemes etc

BDisp commented 1 year ago

@tznind I really don't know, because the CursesDriver call Refresh to write to the screen. In NetDriver we control the writing to the screen and we send escape sequences to a StringBuilder and work well. In CursesDriver maybe is possible to write escape sequences strings to the buffer and perhaps the Refresh can send well them to the screen. But I'm not certain.

tig commented 1 year ago

Based on my research I believe @tznind is right.

I am going to resist trying to do this in my huge pr, so I can get it merged asap, but I'm now convinced we can actually replace BOTH CursesDriver and WindowsDriver with a new "AnsiDriver".

tig commented 1 year ago

Oh, and if we do end up keeping support for 16 bit color, it will NOT be the default.

Based on what I know now, I want True Color to be default with "as smart as possible" fallback to 256 and/or 16.

I worry about performance emitting stuff ourselves. It's easy to make it work, but my intuition is it's also easy to emit way too many sequences potentially causing perf problems.

tig commented 1 year ago

2612 now passes all unit tests - The true color support is fragile though.

I'm going to refactor a bunch of it in line with what I said above in next few days. Hopefully I can get it to a point where it's architecturally correct, if not completely working, in the next few days. I then have a few more issues in #2612 to address before it can be merged.

tig commented 1 year ago

Proposal:

Once the above is done, I can merge to v2_develop.

We can then open PRs to fix/adapt other things:

@BDisp, @tznind, @adstep - let me know if you have more thoughts, questions, or suggestions on all this!

BDisp commented 1 year ago

Did the PR #2729 resolved true color on Linux?

tig commented 1 year ago

Did the PR #2729 resolved true color on Linux?

I don't know. I misunderstood what was going on, and didn't realize it was different set of work than what @adstep did. So I just closed #2729 without looking at it.

Not sure how to handle this now that you've pointed it out.

BDisp commented 1 year ago

Well the PR #2729 includes true color for Linux. I think you must reconsider it.

tig commented 1 year ago

Are you sure it does? I peeked at the code briefly and saw no evidence of related changes to CursesDriver.

I'll look again later.