Replace termbox-go with tcell

[Provessor]

This completely replaces termbox-go with tcell. A few sections needed quite a bit of work because of how tcell constructs colours and events differently. This breakdown hopefully covers everything this would mean for the project, to save extra work by others later.

Hopefully this breakdown contains all of the changes, I have spent quite a while trying to get this as close as possible to the termbox-go implementation whilst modifying the structure as little as possible.

Fixes: #302 and #223 (with configuration)

Breakdown

User Perspective

• The color256 option is gone and a use will receive a warning if color256 or any of its variants is used.
• 24-bit colours are supported.
• Will use a higher colour palette if it is supported (and a wider range of available palettes), without having to enable it -- this may not be desirable but tcell doesn't give an option for this.
• All sub-shells will have $TCELL_TRUECOLOR set to "disable" if it is unset on launch.
• A long preview will no longer write over the status bar when resizing without drawbox.

Output

• tcell requires a context (screen) to print onto, to remedy this a screen is passed around to whoever needs to print.

• tcell operates on styles instead of colours, a style contains a foreground, background and any other attributes the text may have. The foreground/background split has been replaced everywhere with styles instead.

• Some terminals/systems are noted as being not supported, these include

  • Plan9
  • Solaris/Illumos requires CGO
  • mintty and other cygwin style applications

  However, the best I can find about their status with termbox is an issue from ~2015 about them not working.

• I held this off until I had somewhat official documentation for all of the colour parsing sections (instead of just converting the existing code) so I can guarantee any changes I made match the standards.

• Some of the colours are named differently.

Events

• Receiving events is a bit different (always requiring a switch) but there is a richer event interface than with termbox.
• Possibly for the future, tcell has support for some events that termbox did not (most notably mouse events).
• lf now needs to be able to handle EventErrors (such as when the tty goes away), otherwise it will endlessly spin in readEvent() if something goes wrong.
• When the screen is closed with Fini(), asking for an event will always return nil (not error), now all goroutines that handle events will check for this after passing the event down the tree. This lets us close the screen any time without worrying about zombie goroutines. This could be improved to also handle EventErrors but lf will always exit immediately when encountering it. There is an open issue for tcell around the first event disappearing when working with multiple screens, however I haven't encountered this and there is a workaround if it ever becomes an issue. This is also why the screen is closed after app.loop() returns instead of with defer (in case of app.ui.screen changing).

Tests

• New tests for 24-bit colour.
• New tests for colormode limiting modes.
• Now check against styles (int64) instead of comparing the colours.

[gokcehan]

@Provessor Hey, thanks a lot for sending a patch. This seems like a lot of work. We had just made a new release, so it is perfect time for such a major overhaul.

I need some time to understand, review, and test this as I'm still not familiar with tcell. I will be somewhat busy in the next few days, but hopefully I can take a proper look at this in the weekend. I'm not planning to push anything to the repo before this so don't worry about conflicts.

[Provessor]

Here's something I didn't think of before. In the PR I note that;

A long preview will no longer write over the status bar when resizing without drawbox

Which was done here:

lf/ui.go

Lines 194 to 200 in ad533e4

	for i, l := range reg.lines {
	if i > win.h-1 {
	break
	}
	st = win.print(screen, 2, i, st, l)
	}

Could be expanded so that everything lf receives from the previewer will be stored, instead of just what will fit on the current screen.

lf/nav.go

Lines 456 to 465 in ad533e4

	for i := 0; i < nav.height && buf.Scan(); i++ {
	for _, r := range buf.Text() {
	if r == 0 {
	reg.lines = []string{"\033[7mbinary\033[0m"}
	nav.regChan <- reg
	return
	}
	}
	reg.lines = append(reg.lines, buf.Text())
	}

The current state in this PR is probably the most performant but having more of a file shown after a resize would be nicer. This probably belongs in another commit/PR though. Also a custom previewer could handle the situation where requesting more lines is expensive / has high latency such as over a network connection.

EDIT: here is a little patch if you want to apply that after :^)

[gokcehan]

@Provessor I don't resize my terminal often, at least not horizontally, so I was unaware of those bugs and glitches with cached previews. I still think cutting reading early with SIGPIPE is a useful optimization especially for long files. I would be ok to hide this behind an option, though I think it would be better to invalidate file caches with such resize events to fix this issue. It's probably worth discussing this in a separate issue. I appreciate you only stick to porting without anything else.

The patch seems to work fine without any issues, though I wasn't able to test it with unicode rich files as I don't have many of those and I didn't play much with colors either. On windows, the screen flickers with almost all operations for some reason. Windows built was also broken (you need to mirror new definitions in os.go to os_windows.go), so I'm guessing you don't work on a windows machine. I will try to understand why this happens.

I'm not sure if you have looked at it but the only reason we implemented color256 option was to have better default colors as discussed in #104. Normal mode in this patch does not seem to suffer from this issue. Do you think we can get rid of colormode option as well? Is that a leftover from termbox or is there an advantage to supporting different color modes?

Some terminals/systems are noted as being not supported, these include...

It's fine, these are not working with termbox either. As far as I remember, plan9 doesn't have such terminal interfaces, solaris was not building with termbox, and cygwin/mintty are not officially supported by Go. Actually it's more than fine since Tcell already seems to be more portable than termbox so we might be able add new platforms to our cross build system.

Possibly for the future, tcell has support for some events that termbox did not (most notably mouse events).

As far as I know, termbox have mouse events as well, it's just that I never had the chance to work on integrating them in lf.

There is an open issue for tcell around the first event disappearing when working with multiple screens, however I haven't encountered this and there is a workaround if it ever becomes an issue...

I must admit, that issue was a little worrying to read. I'm not against donations but it can become a slippery slope when it is meant to be the main income of someone. I hope we are not locking ourselves to a mandatory donation scheme. On the bright side, tcell is used in popular projects and fixes are more likely, and termbox is not maintained anyways.

[Provessor]

I don't resize my terminal often, at least not horizontally, so I was unaware of those bugs and glitches with cached previews. I still think cutting reading early with SIGPIPE is a useful optimization especially for long files. I would be ok to hide this behind an option, though I think it would be better to invalidate file caches with such resize events to fix this issue.

I assume you mean vertically but yes invalidating caches would probably be the better option.

On windows, the screen flickers with almost all operations for some reason. Windows built was also broken (you need to mirror new definitions in os.go to os_windows.go), so I'm guessing you don't work on a windows machine. I will try to understand why this happens.

My mistake with the definitions, they were sort of a last minute change when I realised that the colours were off on terminals that supported 24-bit colour. When working on this, the only access I have to a windows machine is through a rather slow RDP connection with limited permissions so it makes testing (especially for performance) rather difficult and time consuming. My next focus was going to be having a look at trying to reduce how much lf outputs to the screen. The flickering is probably because ui.draw() is called on every update which starts by clearing the screen.

lf/ui.go

Lines 716 to 721 in ad533e4

	func (ui *ui) draw(nav *nav) {
	st := tcell.StyleDefault
	ui.screen.Clear()
	ui.drawPromptLine(nav)

However this is exactly what is done currently so my only thoughts is tcell is clearing the screen in a different way? The flickering may be reduced by filling the screen with spaces instead of using a sequence? Next week I should have physical access to a windows machine so I could spend a some time trying to remedy this.

lf/ui.go

Lines 682 to 687 in 143de6e

	func (ui *ui) draw(nav *nav) {
	fg, bg := termbox.ColorDefault, termbox.ColorDefault
	termbox.Clear(fg, bg)
	ui.drawPromptLine(nav)

I'm not sure if you have looked at it but the only reason we implemented color256 option was to have better default colors as discussed in #104. Normal mode in this patch does not seem to suffer from this issue. Do you think we can get rid of colormode option as well? Is that a leftover from termbox or is there an advantage to supporting different color modes?

The colormode option was to try and keep part of the color256 option and doesn't really bring any benefit, I couldn't reproduce that issue with tcell but I added it just in case. I can remove it from this PR.

I must admit, that issue was a little worrying to read. I'm not against donations but it can become a slippery slope when it is meant to be the main income of someone. I hope we are not locking ourselves to a mandatory donation scheme. On the bright side, tcell is used in popular projects and fixes are more likely, and termbox is not maintained anyways.

I was also concerned about the result of that issue but tcell seems to be by far the most popular library for this used by a few very popular projects so worst-case it would probably end up maintained by one of them. And as you mentioned some maintenance is much better than none.

[gokcehan]

@Provessor You're right, clearing seems to be different in termbox and tcell, former just clearing the buffer and latter clearing the buffer and flushing the screen. I tried putting space characters as you said similar to the function in termbox compat layer as below:

https://github.com/gdamore/tcell/blob/79a04f1021339fcd7ec20ffac2db31de23c6a946/termbox/compat.go#L134-L142

Flickers are gone with this. However, there is a noticeable slowdown with tcell compared to the termbox version. Even with no clearing tcell seems to be slower than termbox so I think flushing may be the bottleneck. Maybe tcell is not optimized on windows?

[Provessor]

Maybe tcell is not optimized on windows?

That would seem strange given it has a Windows specific draw() which works quite differently to what is used on other systems.
https://github.com/gdamore/tcell/blob/8ec73b6fa6c543d5d067722c0444b07f7607ba2f/console_win.go#L744-L802

It could be worth testing against the other screen type even though the comment states Windows would be unhappier with it.
https://github.com/gdamore/tcell/blob/8ec73b6fa6c543d5d067722c0444b07f7607ba2f/screen.go#L202-L213

Even with no clearing tcell seems to be slower than termbox so I think flushing may be the bottleneck.

IIRC, its been a long time since I've seriously worked with the Windows Terminal but it used to be very slow, especially on scrolling, and didn't even support all of the VT100 sequences. The problem may arise -- like you said -- from the amount of sequences that are sent after a sync. It might help testing against an alternative terminal like ConEmu.

I tried putting space characters as you said similar to the function in termbox compat layer ... Flickers are gone with this.

That seems strange as after looking through tcell, that seems to be what it does on Windows systems
https://github.com/gdamore/tcell/blob/8ec73b6fa6c543d5d067722c0444b07f7607ba2f/console_win.go#L899-L901
And even clearScreen() which is only used internally, just fills the screen with nothing.
https://github.com/gdamore/tcell/blob/8ec73b6fa6c543d5d067722c0444b07f7607ba2f/console_win.go#L912-L931

It is probably not worth me testing any of this until I get physical access so all I can do right now is speculate.

[Provessor]

Also I got an issue about coloured output that doesn't display right with tcell currently. They also mention that it works fine with fzf which also uses tcell so it could be something I'm doing wrong in this patch.

However, in lfp, the preview is weird, it's nowhere near as good as in the shell or fzf, although it's not the same as with 256 colors. What is more weird is that fzf also uses tcell just as lfp does.

[shabahengam]

@Provessor
after running lf(your fork) I see that some colors blinks.
I don't know how you parse LS_COLORS but I know ls doesn't parse 256 colors like that.
here you wrote 5 is always blink but it is wrong.in 256 color mode, 5 doesn't mean blink.256 color codes are like this:

# Text color coding:
# 38;5;COLOR_NUMBER
# Background color coding:
# 48;5;COLOR_NUMBER
# COLOR_NUMBER is from 0 to 255.

check this link for more info
https://www.mail-archive.com/bug-coreutils@gnu.org/msg11030.html

[Provessor]

@shabahengam as you listed in your post, to display 256 colours it is 38;5;COLOR_NUMBER which means it should fall down to: (or equivalent for 48)

lf/colors.go

Lines 111 to 115 in ad533e4

	case n == 38:
	if nums[i+1] == 5 && i+3 >= len(nums) {
	st = st.Foreground(reduceColorToMode(tcell.Color(nums[i+2])))
	i += 2
	} else if nums[i+1] == 2 && i+5 >= len(nums) {

Which skips over the next 2 entries (5;COLOR_NUMBER) so it shouldn't match 5 next time around unless the checks fail.

What does your $LS_COLORS look like so I can try to reproduce?

[shabahengam]

for example this is I have for go files .go 38;5;34;1
if you check that you will see ls show this as bold green (at least in Xterm).
I think the last 1 is the problem.for example I have this for mp3 files .mp3 38;5;191
and if I add 1 to that like this .mp3 38;5;191;1 it will blinks.
PS: I didn't set anything about colormode.I just removed set color256 from my lfrc.

[gokcehan]

@shabahengam @Provessor I think the first check should be i+3 <= len(nums) instead of i+3 >= len(nums). Also maybe it should come before nums[i+1] so it can short-circuit a possible out-of-bounds error.

@Provessor I didn't have a chance to look at this on windows again. I agree windows console is generally slow, though we should not make it slower if possible. I don't know if we have many windows users, but somehow it is the second most popular platform after linux for github downloads, though this may not mean much. Did you have a chance to look at this on windows yet?

[Provessor]

@gokcehan @shabahengam

I think the first check should be i+3 <= len(nums) instead of i+3 >= len(nums). Also maybe it should come before nums[i+1] so it can short-circuit a possible out-of-bounds error.

I would've thought at least the tests could have caught this (the 256 colour case is the same) but it does make sense that it should be the other way around.

@gokcehan

I agree windows console is generally slow, though we should not make it slower if possible

Definitely

Did you have a chance to look at this on windows yet?

I got to have a bit of a look, but not enough to work out why it was slower. I'll write some tests to try and work out if it is intrinsic to tcell or something lf could avoid.

[gokcehan]

@Provessor Today I have been reading tcell code as well as going through issues to see anything relevant to slowness on Windows and I have seen that 24 bit color support for windows is merged yesterday and there is now a mention of a v2 in the readme. I tried v2 with your patch and it still works, though the slowness is still there unfortunately.

I have also been experimenting with different terminals and programs. Among the terminals, windows terminal works the fastest for me, followed by conemu, followed by cmd/powershell. Windows terminal is almost usable but still noticeably slower than termbox. I also tried fzf and micro and I feel like they have the same slowness as well. I think it's just that they don't update the screen as much as lf scrolling through files (e.g. lf feels quite responsive with set nopreview though this may note be desirable).

Maybe there is more coming in v2 in the upcoming days. Perhaps it's a good idea to open an issue about slowness to report our experience as well.

[Provessor]

@gokcehan Thank you a lot for this, I decided to try and dual boot Windows 10 on one of my potatoes this weekend as I haven't been able to get any decent amount of time during the week to test it yet.

If both fzf and micro have the same slowness then I suppose there isn't much we could do except either try to make a fix or wait for a fix at tcell. I have had a look through the tcell source before but couldn't see anything that could be slowing it down so I'll try and get some results about how much slower it actually is and open an issue at tcell about it.

[gokcehan]

@Provessor I was looking at the patch today again, and strangely I don't experience the slowness anymore. I was testing the speed by caching all file previews in the source repo by previewing them once, and then check the time the current file goes from top to bottom when down key is pressed with set wrapscroll option. With slowness, I remember it was taking about 4 secs on cmd.exe. Today, it's about 1 sec, which is on par with termbox.

I don't think I have changed anything in the code. I have upgraded my go version about two weeks ago from 1.13 to 1.15 to work on another issue. Maybe there was something cached in build directories, though I can't reproduce the issue now with older versions. Tcell also had another version 1.4 released along with 2.0, though I tried it now with 1.3 and no issue either. Maybe there was an update to one of the dependencies of tcell. Or maybe I was doing something silly before but I don't know what. fzf still feels kind of slow but I can't compare it now and I'm not sure. Long story short, if I'm the only one experiencing the slowness, I think it's ok now.

By the way, I was looking at this patch today for #451 and it seems that termbox is now broken with wide CJK characters and tcell handles them correctly.

I see you already removed the colormode option. I think the only issue left with this PR is the flickering in Windows. I'm able to fix it by setting all cells to empty as I mentioned before, and it doesn't seem to add any noticeable performance overhead. If you still don't have a Windows machine, I can do this myself after the merge. I was thinking maybe it is better to only add this for Windows as others don't have this issue. This might as well be a bug so we might open an issue about this. As you mentioned, looking at the code, I see that clearing should already be the case. I will try to take a look at this further when I have some time.

We also still need to move envTcellTruecolor from os.go to main.go to fix compilation on Windows.

Lastly, tcell v2 also works without a problem, but maybe it's better to wait until they move from 2.0.0-dev to something stable before we make the switch.

[Provessor]

@gokcehan That's great you aren't having any slowness anymore, strange for it to just disappear randomly though. Which Windows build is this on?

That reminds me, would you like me to add a "Fixes: " list to OP for all the issues that this should solve?

I've put the flickering and envTcellTruecolor into a TODO list for when I have a go at wrangling Windows today, and I would like to at least have a better go at testing it myself on Windows before accepting that it is ready; I should've done that earlier instead of relying on tcell's promises and thinking I checked everything.

I do think it would be better to leave v2 for now, there still isn't much of an indication as to what could change in the future so I would wait at least until a release candidate or equivalent is formed.

[gokcehan]

@Provessor I'm stuck in Windows 1903 for the while. I have been using this for a while so it might as well just be a random bug. I would have been happier if I could have found a way to reproduce it though. I wish this had happened earlier. Sorry for troubling you with Windows.

That reminds me, would you like me to add a "Fixes: " list to OP for all the issues that this should solve?

Doesn't matter much. If you already have the list, you can do it.

You can ping me when you're done.

[Provessor]

@gokcehan I managed to get a handle on a couple of different Windows builds and from my rudimentary tests it seems to have the same performance as r16 using the portable go-1.15 build from golang.org. I'll squash the commits now.

EDIT: for some reason it wants an empty commit to fix merge issues; I'm not skilled enough at git to be able to fix this.

[gokcehan]

@Provessor Alright, it looks good to me, thanks again for the patch.

[gokcehan]

@Provessor Strange things are now happening again after the merge. I tried upgrading my installed version with go get -u github.com/gokcehan/lf and this is what I get now:

Colors are gray and the current file is not highlighted on gray items.

Then I tried downloading the repo to the desktop and then build it from there. This way, colors are working.

However slowness is back again in both cases. I now recorded a screencast to show what I mean:

https://imgur.com/a/ByjEXdA

(Let me know if the video link doesn't work)

This is with down key pressed and hold on cmd.exe. I think screen recording makes it a little bit slower but it's not fast without the recording either (e.g. takes about 4 secs from top to bottom when files are cached).

I will post updates here if I can figure things out. Ideas are welcomed.

[gokcehan]

@Provessor I tried clearing the cache with go clean -cache -modcache -testcache -i -r which didn't help. Then I tried removing all packages in my ~/go folder before clearing cache, which didn't help either. Then I tried to restart my computer and speed is back to normal. This is quite frustrating.

I'm still looking for a solution for the gray color issue.

[shabahengam]

I'm completely newbie in golang but I think something is wrong in go.mod and go.sum
in go.mod we have go 1.12. maybe change that to latest version?
after go build , git telling me that go.sum get changed.why go-runewidth v0.0.4 get added to go.sum ?
also after go get -u some modules get update like tcell and sys

this is git diff go.sum after go build

index b94b2e5..7b3f958 100644
--- a/go.sum
+++ b/go.sum
@@ -1,10 +1,16 @@
 github.com/DATA-DOG/go-sqlmock v1.3.3/go.mod h1:f/Ixk793poVmq4qj/V1dPUg2JEAKC73Q5eFN3EC/SaM=
+github.com/gdamore/encoding v1.0.0 h1:+7OoQ1Bc6eTm5niUzBa0Ctsh6JbMW6Ra+YNuAtDBdko=
 github.com/gdamore/encoding v1.0.0/go.mod h1:alR0ol34c49FCSBLjhosxzcPHQbf2trDkoo5dl+VrEg=
+github.com/gdamore/tcell v1.3.0 h1:r35w0JBADPZCVQijYebl6YMWWtHRqVEGt7kL2eBADRM=
 github.com/gdamore/tcell v1.3.0/go.mod h1:Hjvr+Ofd+gLglo7RYKxxnzCBmev3BzsS67MebKS4zMM=
+github.com/lucasb-eyer/go-colorful v1.0.2 h1:mCMFu6PgSozg9tDNMMK3g18oJBX7oYGrC09mS6CXfO4=
 github.com/lucasb-eyer/go-colorful v1.0.2/go.mod h1:0MS4r+7BZKSJ5mw4/S5MPN+qHFF1fYclkSPilDOKW0s=
+github.com/mattn/go-runewidth v0.0.4/go.mod h1:LwmH8dsx7+W8Uxz3IHJYH5QSwggIsqBzpuz5H//U1FU=
 github.com/mattn/go-runewidth v0.0.9 h1:Lm995f3rfxdpd6TSmuVCHVb/QhupuXlYr8sCI/QdE+0=
 github.com/mattn/go-runewidth v0.0.9/go.mod h1:H031xJmbD/WCDINGzjvQ9THkh0rPKHF+m2gUSrubnMI=
+golang.org/x/sys v0.0.0-20190626150813-e07cf5db2756 h1:9nuHUbU8dRnRRfj9KjWUVrJeoexdbeMjttk6Oh1rD10=
 golang.org/x/sys v0.0.0-20190626150813-e07cf5db2756/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
+golang.org/x/text v0.3.0 h1:g61tztE5qeGQ89tm6NTjjM9VPIm088od1l6aSorWRWg=
 golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
 gopkg.in/djherbis/times.v1 v1.2.0 h1:UCvDKl1L/fmBygl2Y7hubXCnY7t4Yj46ZrBFNUipFbM=
 gopkg.in/djherbis/times.v1 v1.2.0/go.mod h1:AQlg6unIsrsCEdQYhTzERy542dz6SFdQFZFv6mUY0P8=

[Provessor]

@gokcehan Very strange that it goes away after a reboot.

I had another look through the issues for tcell mentioning "Windows" and none of them mention any issue like this so there is a chance it is just on your machine or build (most of my testing was on 1809 LTSC, none of it on 1903). Probably worth opening an issue at tcell about this but if you can't reliably reproduce it then that might not help much.

Also it seems like the maintainer gdamore runs Windows 10 at least some times so I doubt this would be a widespread issue.

[Provessor]

@shabahengam Yes, that could be updated but it is more about minimum requirements.

In your diff the lines without /go.mod aren't added anymore with go 1.15 and version 0.0.4 of go-runewidth is added because it is still on your system and go build sees that it is compatible (go doesn't remove old versions after an update).

[gokcehan]

@Provessor I figured out the color problem. I had assumed go get would run in module-aware mode with GO111MODULE=auto since it wouldn't be possible to run this command inside the repository without downloading it first. I was wrong. So setting GO111MODULE=on before go get installs a proper build without the color issue.

I have reproduced the slowness a few more times. It happens when I install a new version of the binary and goes away after a reboot. I think you're right that it may be a Windows problem but I can't upgrade to the latest version at the moment. In any case, rebooting seems to fix the problem so if anyone else experiences the same issue we can simply recommend rebooting.

@shabahengam As @Provessor said, that version is about the minimum, and I have already bumped our travis version to 1.15 recently. Having a dirty repository may prevent updates so it can be annoying. I have seen in the documentation that there is a flag mentioned -mod=readonly that you can use to avoid changing the go.mod file and maybe also go.sum file as well, though I still don't know much about modules.

[Provessor]

@gokcehan great, those might be worth documenting in the wiki or somewhere else easier to find.

[mohkale]

@gokcehan definitely agree that this should be in the output of lf --doc or somewhere close to it. Thnx for linking me to this issue.

[gokcehan]

@mohkale I have now updated the source build instructions in the readme and the tutorial to include GO111MODULE=on.