Feature/swap to tcell/v2

[dankox]

This is a try to implement tcell/v2 instead of termbox in gocui. I used the tcell/termbox package as a starting point and then adjust it to better reflect what is used in gocui. I'm not sure if that is fine, the code in tcell is under Apache license and I adjusted it. Hoper that's fine if it's done in open source project.
True color support was added (which was my main reason I looked into this, termbox on windows supports only 8 colors) and mouse support implemented.

The code is done with attempt to be compatible with the previous version of gocui. However, internal stuff were changed a bit.

Colors are starting from 0. I used the original tcell/v1 versions of colors. Colors from 0-255 are just numbers. If RGB colors are used, user need to add gocui.AttrIsRGBColor flag to the gocui.Attribute to specify that this is RGB color. This shouldn't be breaking change for most gocui applications, as the original color variables are still the same. If somebody was creating colors like gocui.Attribute(ansicolor+1), then they would have to do changes to the code because now the colors starts from zero.
Afterwards, when calls to tcell/v2 are done, colors are adjusted to work with v2 (tcell.ValidColor flag added to them).

Mouse clicks should work more or less the same way. tcell doesn't have the Key for mouse buttons, so I tried to translate it in a way, so users don't have to change their keybindings. It works for simple clicks (left, right, middle, wheelup/down) from what I tested.

All the example programs are working for what I could test and I added one more example to test 24bit colors.

However, tests were done only on remote Linux machine (accessed from Windows Terminal), where everything worked well (even the mouse). On Windows 10 it worked too, but mouse example doesn't work for me. However, mouse example never worked for me even before, so I'm not sure if it's not because of my terminal or some other setup in my Windows (funny that mouse worked on win7, but not 10 for me). All the other examples work.

Windows 7 is a bit problem. Still looking into it if it's possible to fix. Didn't try it after I swap to v2. In v1 it rewrites the screen weirdly (kinda shift updated stuff one row down).

Additional tests on other platforms are welcomed, and if anybody see any problem, let me know, I will try to look into it.

Edit: This PR is based on #72 (so there might be more changes visible). Hope that's fine. I can rebase if necessary.

[mjarkk]

ran all the examples on linux and found a few issues:

• In custom_frame.go the overlap seems a bit wired:

• In goroutine.go and table.go the space between the numbers has a black background: (A comment in the original pr contained a fix)

[dankox]

@mjarkk Thanks...

1. custom_frames.go This example was created to demonstrate the rune changes and also how overlap works. I didn't really change overlap function, the reason why it is like that, is because I setup views in such way.
   The v2 overlap doesn't have all the runes necessary for creating a correct corner. Again, for demonstration and test that it can fall back to original runes when FrameRunes doesn't have correct length (contain all runes).
   The v4 overlap is moved one column to the right, to demonstrate what actually mean the setting of overlap and the overlap byte in SetView.
   If you run that example with original gocui, you should see something similar (without colors and double edges though). I can make that example to look more clean. I only tried to set it up so it's more obvious what overlap does and test the edge cases.
2. goroutine.go Thanks for pointing this out. I saw that comment before, but I never see the issue, so I wasn't sure what it is about. I will do the changes.

[dankox]

Fixed goroutine.go and table.go example in eaa6f5a.

This commit changes how colors are handled. In tcell/v1 default color was -1. They changed it to 0 in tcell/v2. Before I used the original concept, but I changed it to be compatible with tcell/v2 coloring system. You can wrap those colors without any necessary change to it.
The line with append of default cell{} had all the values zero, which wasn't default before. Now it is again.

The fixColors function was updated to make "old" colors work with the new system. If any color is in range 1-256 and not marked valid (by the new system), it's from the "old" version and will be translated into 0-255. Also the different output formats are using the exact same handling as termbox-go has, so applications using the old coloring system should appear unchanged.

I might move all the colors and attribute to separate file attribute.go (which was removed) later on, as it is starting to be bigger. Wanted to create some interface on top of it, so it's easier manageable. Any advice or suggestions on that are welcomed :)

[mjarkk]

Once this is dune we probably want to release this as a v1.0.0 release so people understand this is a large change.
Maybe we should add a section to the readme or an extra markdown file with documentation for migrating from v0.x.x to v1.0.0

[dankox]

I agree. I was thinking the same, that if it shouldn't be marked with some version (similar like tcell has version 2 now), but I don't really know how that works. If tag is enough and go can find out or what :) Need to look into that a bit.

Documentation is definitely good idea, to point out what kind of changes are under the hood, so people could decide how big is the migration. Btw I was trying some gocui application (git-bug) and seems like it work, so I hope migration could be smooth.

Figured out problem with Windows 7 and tcell. I don't think it needs to be addressed in the code base. It could be just documented.
When tcell updates screen buffer thru Windows Console API, it does it with WriteConsole call which writes output and move cursor. On ConEmu and maybe some other terminals, what this does it moves to the new line (because the full row was displayed) and cause a scroll by one line.
This can be resolved by setting environment variable TCELL_TRUECOLOR to disable or the ConEmu variable. tcell would set basic mode for the console and will not cause this problem.

[mjarkk]

Btw I was trying some gocui application (git-bug) and seems like it work, so I hope migration could be smooth.

That's awesome

Figured out problem with Windows 7 and ...

I don't think there are a lot of gocui users on windows let alone windows 7 so i wouldn't worry about it to much.
Most people using TUIs are probably also using Macos or Linux and this is usually reflected by the downloads on the releases page of TUI programs.

This can be resolved by setting environment variable TCELL_TRUECOLOR to disable or the ConEmu variable. tcell would set basic mode for the console and will not cause this problem.

Maybe add this to the migration docs

[MichaelMure]

Awesome job guys :)

[dankox]

@mjarkk I've moved the functions around and made all the tcell stuff private. Also I changed that tcell_compat.go into tcell_driver.go and make it more compact with stuff which are necessary only.

attribute.go - I return Attribute here and add some more "methods" to it so user can easily creates colors or access them. keybindg.go - moved the keys here and fixed space and ctrlspace.

Hope it's fine... let me know what you think or what should be changed.

[mjarkk]

It's looking great!

Very nice documentation file about the changes between termbox and tcell!
We might want to mention it somewhere at the top of the readme, it's currently not very clear it exists.

[dankox]

Thanks.

Oh yeah, definitely... wasn't sure where, so I just created separate file which could be referenced anywhere. Feel free to include it where it might fit. I'll fix the missing gocui references.

[mjarkk]

Hmm changed my mind after seeing all these prefixes i think it only bloats the output.
I guess everyone directly knows what we mean with these names and where to find them.

I've pushed a commit to remove all the gocui. prefixes

[mjarkk]

@glvr182 any suggestions for this PR, i think this is almost ready to be merged?

[dankox]

@mjarkk one more thing I realized. I didn't add the Windows 7 TCELL_TRUECOLOR=disable "trick" into the documentation, because it was actually fixed in tcell in the latest commit. However, it's not included here, because the go.mod points to tcell/v2 v2.0.0 version. I didn't update it as I didn't know if go.mod pointing to specific commit instead of tag is ok.
If you would like to add it, it would be something like this

github.com/gdamore/tcell/v2 v2.0.1-0.20201109052606-7d87d8188c8d

So let me know and I can update go.mod with the latest tcell@master.

[dankox]

@mjarkk another thing :) which I forgot to mention. I guess you saw it in code, but still want to point it out.

https://github.com/awesome-gocui/gocui/blob/d20d4521a849df96f7b2df864d17ff374b442fc2/gui.go#L521-L523 Here is event handler, and I added there in comment eventResize. However, I wasn't sure if it needs to be implemented (it's quite easy, just a screen.Sync()).
In original, it wasn't there (even though termbox had it too), so I didn't add it. It was acting weird sometimes. But mostly on my old Windows 7. So not sure, if it should be enabled or not. Just so you know about it.

[mjarkk]

So let me know and I can update go.mod with the latest tcell@master.

I think we should not go with the latest master branch if they make releases, though as you pointed out ...re/tcell/v2 v2.0.1-0.2... also seems to be a valid release so yea lets go for that.

In original, it wasn't there (even though termbox had it too), so I didn't add it. It was acting weird sometimes. But mostly on my old Windows 7. So not sure, if it should be enabled or not. Just so you know about it.

Our current solution seems to work fine as you reported it to work on windows and in my case it working perfectly fine on linux.
So i guess we should keep it the way it is currently.
After doing a major release we will see if there are issues with this and from there we can investigate if we should maybe change this though i don't think there will be any.

[dankox]

Our current solution seems to work fine as you reported it to work on windows and in my case it working perfectly fine on linux. So i guess we should keep it the way it is currently.

Sounds good.

I think we should not go with the latest master branch if they make releases, though as you pointed out ...re/tcell/v2 v2.0.1-0.2... also seems to be a valid release so yea lets go for that.

Sounds reasonable, that's why I asked :) About the version, it's "pseudo-version", it's not the real version. Last published version is v2.0.0. This one was created by Go, when I did go get github.com/gdamore/tcell@master or something like that. Basically it's a version pointing to specific commit in master.
So I would assume, we would keep the v2.0.0 version as is instead of that "pseudo-version" mentioned above.

[glvr182]

@dankox wow... Thank you so much for putting this together, this is amazing! Also @mjarkk thank you for doing the reviews, I had finals and didnt check gh in a while because of it.

A couple of questions on my end:

• What about mouse support now, does it still work? I ask because I saw you assigned left/right mouse to random values
• How much work did it take for you to migrate git-bug from old to new gocui
• If we were to merge this into master and release this as v1, could that be received badly. I image there are a lot of breaking changes in this haha. IF it breaks a bunch of things, then we could consider releasing it in a subfolder (v2) and publish that.

[dankox]

@glvr182 thanks a lot... to be honest the primary drive was not enough colors in gocui/termbox for my application on windows :)

Really sorry for the wall of text, but I hope it clarifies most of the stuff.

1. Mouse support

Mouse support should work, more or less. I had limited testing capabilities as I could test it only on Windows 10 and 7 and remote linux machine (still using Windows Terminal) and the testing application was the one provided in _examples/mouse.go.

~I "copied" the way how tcell has it in their examples, so basically whenever a button is hit, it should be stored as last button and when no button is hit (something like ....~ I'm sorry, I didn't understand the question about random assignment before. So the previous text was not to the point.

As for random assignment. I used the original idea from tcell/termbox package, where they assigned keys which are not used to MouseLeft, ... I kept it that way, because I cannot map it to the same value as in termbox. This is due to termbox having Key as uint16 while tcell has Key as int16. Mouse buttons were around xFFFF area, which cannot be converted to int16. Or at least I'm not sure how, it throws me error that it overflows.
This might be a breaking change if somebody creates Key with the original value instead of gocui.MouseLeft variable. I mentioned something similar lower, but I forgot about the keys underlying type.

2. Git-bug migration

To be honest, all the migrations I tested were that I just swapped gocui in go.mod. It worked right away. I tried to test some stuff, but I didn't really go deeply thru the application. Mostly I was trying if keys work or if there is some problem with displaying and stuff.
I know that @MichaelMure was mentioning in original PR to test with asian runes. I didn't really do as I didn't really know what exactly. As far as I know, gocui might still have this problem (I know git-bug has its own way of dealing with it, so I don't think this should be affected), but @mjarkk was working on it in different PR.

I also tried some other applications, which I found referencing this repo, like https://github.com/wagoodman/dive
This application is built on top of original gocui, they were mentioning this repo because of tcell migration (some user wanted them to migrate it to tcell too). This actually made me change how spacebar was treated at first, because they have some keybinding parser (not the gocui parser, with that it would work right away) which allows configuration and because space is originaly Key not rune I changed it (tcell reports it as KeyRune).
As for migration, I just changed go.mod similarly as in git-bug and had to update the SetView and NewGui as the original one doesn't have the overlays setup in them. Btw... was thinking this could be removed as it's accessible thru public members as all the other setup. But that's not the point of this PR. I remember seeing discussion about it in some issue :)

3. Breaking changes

This was something I was coming back mainly because of the colors :) tcell treats them differently and at first I implement it in the same way as tcell does. However, I returned back to my application and realized that I was using Attribute in "termbox" way, meaning colors are always +1. It wasn't a big change in my application, but I realized that if anybody will be doing something similar, they might need to do changes which are not so obvious.
So in the end, I insert there backward compatibility for this, so all the original application should have the same colors as they were using and if you want to leverage the newly added OutputTrue, you can and update your coloring system in application by using the provided functions like Get256Color or GetRGBColor which returns Attribute with correct value.

Events like keys or mouse, might breaks something, but I was trying to test most of the cases which I could think of and was trying to make it non-breakable. I could imagine somebody might find some breaking changes in this, however I think it will be more like a bugs than non-fixable breaking changes. But this is just my opinion. It's hard to tell as the range of terminals and platforms is big. So who knows :) Would be good if people could test it.

All in all, I hope there is no "high-level" breaking change at all. That was my original intention with this, to make it work as is and people could just do go get gocui@latest and be fine with all the new support.
However, internal representation of some stuff like Attribute was changed and if somebody was doing some arithmetic with it, this wouldn't work anymore (or need to be adjusted). I tried to cover it in the CHANGE document.
One other thing which comes to my mind is, performance. It's questionable if tcell has different performance than termbox and how it runs. I had no problem so far, but I remember reading in original PR somebody reporting "performance" problem with resizing (lagging or so).

[MichaelMure]

Regarding git-bug's "own way to deal with asian characters", it's only limited to how text is formatted and aligned. That is, where line breaks are added and so on (see https://github.com/MichaelMure/go-term-text). As far as gocui is concerned, there is nothing special going on. I mentioned that in some issue because 1) it's a good thing to test and git-bug does support it and 2) it was broken in gocui at some point.

Btw... was thinking this could be removed as it's accessible thru public members as all the other setup. But that's not the point of this PR. I remember seeing discussion about it in some issue :)

You have my (worthless) vote!

Again, thanks for working on this :)

[dankox]

@MichaelMure I see... I guess that shouldn't be really affected as tcell works with "cells" similarly to termbox. I tried to paste in the editor some Japanese text and when I saved it, it showed up correctly (from what I could see).

Thanks :)

[glvr182]

Thank you for the detailed information! This helped me a lot. 1: We would have to do some testing on the linux and mac platform. I can do linux but not mac 2: Thats good to hear, we can ask some developers using the default Gocui about testing it 2.5: We could indeed remove the overlay param in NewGui and SetView, multiple people informed us that it is confusing and since this would be a big change anyway, now could be the time do so. 3: The main reason we also want to change to Tcell is because of termbox not being under active development anymore, i spoke to the dev of termbox and he basicly told me, use tcell

All in all this PR looks really good, I'm very thankfull for what you have done! I hope we can do some testing on both fake projects (examples and such) and real projects (git-bug, dive, lazy*)

btw @MichaelMure your vote is worth the same as mine, dont undersell yourself

[dankox]

@glvr182 I totally understand the 3rd point. As I mentioned before, I had a problem with tcell on windows 7, which when I opened an issue was resolved right away, so I can see that as huge benefit as tcell is quite actively maintained.

I was looking into testing lazy* projects before, but @jesseduffield is using his own version of gocui which is a bit more customized with context, special key handling and whatnot. There is quite a bunch of work to just merge these two repos together unfortunately :/ so I skipped it in the end.

[MichaelMure]

btw @MichaelMure your vote is worth the same as mine, dont undersell yourself

I'm not underselling myself, but free software is pretty much a do-ocracy: the one doing something should decide what's best. I can't just do nothing and tell people "you should not do that" ;)

[glvr182]

@dankox Yes, @jesseduffield added a lot of features to his fork, we might want to start implementing those in this fork as well (after this is merged of course).

This PR has my vote, but i do want to do some small testing.

@mjarkk Whats your vote

@MichaelMure fair point! Just making sure :)

[mjarkk]

So heads up i'm kinda busy last week and will be for the upcoming week too.
I'm fine with this getting merged.
It's not that people directly use the latest version when we create a new release they need to specifically upgrade to the latest version so if there are still things that do not work we'll start to see some issues then.

[mjarkk]

Just got my hands on a macos device and so i tested all the examples.
Everything expect from the true color example seems to work fine.
The output on my screen with the true color example:

This is on the default MacOS terminal no modifications made.

Some env vars that might have impact on this:

LC_CTYPE=UTF-8
PWD=/Users/mark/Documents/gocui/_examples
SHELL=/bin/zsh
TERM=xterm-256color
TERM_PROGRAM=Apple_Terminal
TERM_PROGRAM_VERSION=440

[dankox]

@mjarkk Does that terminal supports 24bit colors? I think you need at least iTerm3+ to get the colors. The default one had only 256 color support.

Can you test some colors in it? Like this: printf "\x1b[38;2;202;150;34mTRUECOLOR█\x1b[0m\n"

To be honest I just made up that sequence, so feel free to adjust for your test.

[mjarkk]

You are correct, with iterm it works like expected.

[mjarkk]

@glvr182 pinging your to check if we can merge this in.
I currently have a vacation and in these corona times i have lots of time to fix bugs if they might occur after merging.
How about we create a beta release so we can still change things after merging this.
I would hate to see this stay here forever :)

[jesseduffield]

I’ll be happy to try my luck at getting this to work with lazygit when merged, awesome work!

[glvr182]

@ all sorry for my inactivity, had a bit of a development dip, I'm sure most of you know what I'm talking about. I'd be up for a beta release, see how things go and get some parties to try it out!

[mjarkk]

@glvr182 Totally understandable and relatable.
Can you approve the changes so we can merge this without clicking red buttons lmao :)

[dankox]

Great, I'm glad this is moving forward. I have implementation of this in my application, which is still private, but when it will be published/public I let you know. For me it works in my use-case :) Looking forward to see if, there are any problems which need to be fixed or if it's good as is.

[mjarkk]

Great work @dankox i have merged this, the circle ci seems to fail building now i'll check that and after that create a beta release.
Oh and @dankox if you like the project and want to keep it alive you are probably also welkom as org member :).

[dankox]

@mjarkk I saw that... it's the gofmt on custom_frame.go example and attribute.go.
I think I found the problem... it's probably because of CRLF lines in those file. All the other files have LF. I guess I screwed up when I was adding those files as I did it on Windows where I have core.autocrlf turned off. I forget to change the line ending to LF.

Maybe if you could push directly to the master... or I could create a quick PR for that one.

[mjarkk]

It seems like, i've fixed the issues in: https://github.com/awesome-gocui/gocui/pull/74

[mjarkk]

The release is up: v1.0.0-beta-1

[dankox]

Great thanks...

[mjarkk]

Also just created an issue in tcell to ask if they can add us to the examples https://github.com/gdamore/tcell/issues/417