Update tcell to the latest 2.x version

[mum4k]

Changelog: https://github.com/gdamore/tcell/blob/master/CHANGESv2.adoc

And while we are at it, we can also consider making tcell the default in all code examples.

[mum4k]

@dyc3 I got some prep work done (making tcell default, updating documentation), but didn't get to create the PR that updates to 2.x yet.

The next steps should be something like:

1. update the dependency
2. update all import paths
3. verify that none of the breaking changes actually break our build and functionality. The only one that looks worrying is the change of mouse button handling. We may need to use or modify one of the demos to verify the new behavior.

If you have the time to prepare the PR with at least the first two steps (and anything I might have missed), that would help a lot. Alternatively I can get to it a few days from now. I will be of course happy to help with the testing step, maybe we can both poke at it - more eyes gives us a better chance of catching anything that broke.

[mum4k]

@dyc3 if you choose to work on the PR, please fork from the devel branch to avoid conflicts and include the latest changes we made.

[dyc3]

I'll take a look at it tomorrow. It's currently 3 am where I am and I need sleep haha

[mum4k]

We seem to be in the same timezone, so I can relate...

And thank you of course.

[dyc3]

So far I've only updated the import path for tcell to reference v2, and all the tests pass. Seems like a good sign.

[mum4k]

While that is good news, it is likely that it only gives a false sense of security. The unit tests in termdash only verify its own logic and only reach up to the terminalapi. The terminal drivers themselves are abstracted behind a fake terminal implementation in the tests.

If I remember correctly, we don't have a single integration test with either termbox or tcell, so the only way to tell if this works correctly may be by running the demos and exercising the functionality.

[dyc3]

Color is broken because the number system was changed. Font modifiers still work though.

colors.go diff of 1.4 and 2.0:

1c1
< // Copyright 2015 The TCell Authors
---
> // Copyright 2020 The TCell Authors
23a24,26
> // We use a 64-bit integer to allow future expansion if we want to add an
> // 8-bit alpha, while still leaving us some room for extra options.
> //
28c31
< type Color int32
---
> type Color uint64
32,33c35,41
< 	// system or teminal default may exist.
< 	ColorDefault Color = -1
---
> 	// system or terminal default may exist.  It's also the zero value.
> 	ColorDefault Color = 0
>
> 	// ColorIsValid is used to indicate the color value is actually
> 	// valid (initialized).  This is useful to permit the zero value
> 	// to be treated as the default.
> 	ColorValid Color = 1 << 32
38c46,50
< 	ColorIsRGB Color = 1 << 24
---
> 	ColorIsRGB Color = 1 << 33
>
> 	// ColorSpecial is a flag used to indicate that the values have
> 	// special meaning, and live outside of the color space(s).
> 	ColorSpecial Color = 1 << 34
45c57
< 	ColorBlack Color = iota
---
> 	ColorBlack = ColorValid + iota
820a833,839
> // Special colors.
> const (
> 	// ColorReset is used to indicate that the color should use the
> 	// vanilla terminal colors.  (Basically go back to the defaults.)
> 	ColorReset = ColorSpecial | iota
> )
>
971a991,1000
> // Valid indicates the color is a valid value (has been set).
> func (c Color) Valid() bool {
> 	return c&ColorValid != 0
> }
>
> // IsRGB is true if the color is an RGB specific value.
> func (c Color) IsRGB() bool {
> 	return c&(ColorValid|ColorIsRGB) == (ColorValid | ColorIsRGB)
> }
>
975a1005,1007
> 	if !c.Valid() {
> 		return -1
> 	}
977c1009
< 		return (int32(c) & 0xffffff)
---
> 		return int32(c) & 0xffffff
995a1028,1040
> // TrueColor returns the true color (RGB) version of the provided color.
> // This is useful for ensuring color accuracy when using named colors.
> // This will override terminal theme colors.
> func (c Color) TrueColor() Color {
> 	if !c.Valid() {
> 		return ColorDefault
> 	}
> 	if c&ColorIsRGB != 0 {
> 		return c
> 	}
> 	return Color(c.Hex()) | ColorIsRGB | ColorValid
> }
>
1004c1049
< 	return ColorIsRGB | Color(v)
---
> 	return ColorIsRGB | Color(v) | ColorValid
1019a1065,1069
>
> // PaletteColor creates a color based on the palette index.
> func PaletteColor(index int) Color {
> 	return Color(index) | ColorValid
> }
\ No newline at end of file

[mum4k]

Thanks for catching that, I do remember reading something about color palette changes in the v2 changelog. We may need to update our tcell library to do any necessary translation.

Let me know if you get tired of looking at it, I can check out your branch and try to propose some solutions.

[dyc3]

I'm kinda stumped.

I think fixColor needs to add tcell.ColorValid to the color value? When I try that, I get colors back, but they are all wrong.

And the tests for cellColor and fixColor are still passing, which doesn't really make sense because I'm changing the behavior of both of those functions...

[mum4k]

I will take a look a bit later tonight and update here. I need to spend some time understanding the change they did to v2.

[mum4k]

I think I understand the problem now and I am fairly convinced this is a bug in our tcell library. I will send you a PR branched off your branch that will fix that and also align our color definitions with the standard tcell is using. Since that is the official standard. I just need to figure out how to make this backward compatible for the termbox users out there.

[mum4k]

The following PR is addressing the color problem: dyc3#1

The issue was that our tcell library was depending on the values of tcell's color constants. We shouldn't have done that, those are internal to their implementation and they did change them when upgrading to v2.

Feel free to merge these commits into your PR, we can push it into the devel branch altogether.

Did you find any other incompatible or breaking changes? Any concerns about the change related to the middle mouse button?

[dyc3]

I'll go ahead and merge that in.

I haven't tested the mouse button stuff yet. I'll look at it in a bit, I'm currently fighting fires on another project.

[mum4k]

Take your time and feel free to let me know if you would like me to take point on this.

[dyc3]

I've tested mouse inputs on Linux, and they work as expected. I don't have an easy way to test on windows currently.

The function keys also work as expected, however modifiers are not passed along at all. Is this intentional?

[mum4k]

Thank you for testing it, I will give it a go too to confirm later today.

Can you give me a few more details about the modifiers? Do you mean that some of our keys don't work, e.g. KeyCtrlA:

termdash/keyboard/keyboard.go

Line 59 in 2a7dafa

KeyCtrlA: "KeyCtrlA",

Or do you mean that the new support for modifiers added by tcell in v2 isn't reflected in our API?
https://github.com/gdamore/tcell/blob/7d87d8188c8d3a236c778107d4925a9b2465a5f6/key.go#L73-L79

In other words - did any existing functionality change after upgrading tcell to v2 or are we just missing new functionality?

Additionally is #260 ready for review or do we still need to add something?

[dyc3]

Oh wait, I see.
The keys like KeyCtrlA work, but Shift+F1 gives KeyF1.

We're just missing new functionality. #260 is ready to review

[mum4k]

Yes that would be a missing functionality and is tracked in #262.

The PR looks good, please let me know if you are planning to send one more PR to add italic and strikethrough before we push the release.

[mum4k]

Resolved by #266.