Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to tcell #45

Closed
wants to merge 15 commits into from
Closed

Switch to tcell #45

wants to merge 15 commits into from

Conversation

mjarkk
Copy link
Member

@mjarkk mjarkk commented Oct 19, 2019

Still a lot of work to do to see if it's possible to replace termbox with tcell but were getting somewhere.

Known issues/todo:

  • Some spaces created by gocui are rendered with a black background (see goroutine.go)
  • Windows
    • colors written to the view not working (see colors.go)
    • Resizing the terminal doesn't seem to update the ui (see size.go)
  • Need to import tcell to use keybindings it would be nice if there is a full copy of all keybindings in gocui
    • Also add extra aliases for the KeyUp, KeyRight, etc to KeyArrowUp, KeyArrowRight, etc this will reduce the amount of work required for a end user when updating this library or migrate to this library

Examples tested and fully working:

  • active.go
  • bufs.go
  • colors.go
  • colors256.go
  • demo.go
  • dynamic.go
  • flow_layout.go
  • goroutine.go
  • hello.go
  • layout.go
  • mask.go
  • mouse.go
  • ontop.go
  • overlap.go
  • size.go
  • stdin.go
  • table.go
  • title.go
  • widgets.go
  • wrap.go
  • keybinds.go

If this pr gets to a stage where it can do the same as the current master and we all agree on using this i also want to try to update a real project to see if this is actually production ready and see if there are any left over bugs.
Projects i would like to test this on:

  • lazygit

The go mod and sum are renamed because go 1.13 has some wired issues.

There are still some background color issues left but this has helped a fair bit to reduce it

When loggint the char rendered to the screen it's background color sometimes the color is 0 instaid of the default tcell color (-1) and thus the background of the char is black instaid of the default color.

In termbox the default color number is 0 and thus all struct values are left empty because there is no point to setting 0 but in this case that's a bit annoying because tcell's default color is diffrent
The char is not nil if the tcell.Key is defined
@glvr182 glvr182 added enhancement New feature or request WIP Work In Progress labels Oct 30, 2019
"os"
)

func l(v ...interface{}) {

Choose a reason for hiding this comment

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

func l is unused (from unused)

@mjarkk mjarkk mentioned this pull request Nov 23, 2019
@MichaelMure
Copy link

Just a suggestion, https://github.com/MichaelMure/git-bug might be a good real world test as well, especially if you write some comment in wide character like with japanese or chinese.

@abitrolly
Copy link

Why use tcell over termbox in the first place?

@mjarkk
Copy link
Member Author

mjarkk commented Nov 26, 2019

@abitrolly see #42 for the main reason
TL;DR: There are some bugs in termbox that limit gocui in what i can do so we are trying to migrate to tcell to see if that can fix all the problems we have ow and termbox is dead and tcell is maintained.

@glvr182
Copy link
Member

glvr182 commented Dec 2, 2019

I am stronly against using a vendor folder in this project. I think using a vendor folder adds unneeded amounts of code to the library and defeats the whole purpose of using modules (for this library)

Edit: this was based on an older version, i see you have removed it

@glvr182
Copy link
Member

glvr182 commented Dec 2, 2019

I can help you with the files if you want, just give me a file to change and ill work on it

@mjarkk
Copy link
Member Author

mjarkk commented Dec 2, 2019

Thanks @glvr182
I'm really busy this week so i don't think i will be able to push anything so you can do whatever you want :).

Something that i cant seem to fix is in the goroutine example where there are black boxes between the numbers, if you want to help with that that would be great.
Also the colors example has different colors then the current state of gocui i'm also not sure why that happens.
Ow and if you have access to a windows PC i'm not sure but from what i can remember resizing didn't seem to work.

About the vendor folder..
I can't seem to get go modules working smoothly in vscode so i found this "hack" where i force all go tools in vscode to use the old school vendor folder but this requires me to always have a vendor folder.
The only thing is now that i need to not commit the vendor folder what i've already dune a few times.

@glvr182
Copy link
Member

glvr182 commented Dec 2, 2019

We can hop on a call if needed so i can help you with the gomodules if you want.
I'll check if i can fix those examples for you.

@glvr182
Copy link
Member

glvr182 commented Jan 18, 2020

Ill work on this issue since there seems to be some demand for it @mjarkk. As for your vscode issue have you checked this page?

@mjarkk
Copy link
Member Author

mjarkk commented Jan 18, 2020

Thanks for trying to continue my work.
For the my vscode issues, Yes i have and it's a mixed bag. For smaller projects like this it usually works "fine" but when working on big project what i do at work or when i have multiple projects open at the same time it has some supper annoying quirks.
Sometimes it seems to get stuck and then it will just use 100% of the CPU for a long time or keep eating ram to the point where other programs get moved into swap.

@MichaelMure
Copy link

Highly recommend Goland as an IDE for go. You can start with the 30 days trial and apply for a free open source license: https://www.jetbrains.com/community/opensource/

@glvr182
Copy link
Member

glvr182 commented Jan 18, 2020

For students GoLand is free to use I think, so if you want to try that, deff give it a go! As for VSCode yea there are still some issues with Go. Usually restarting the language server works for me.

@glvr182
Copy link
Member

glvr182 commented Jan 18, 2020

Testing these now, will push results later.

  • bufs.go // Just some tcell imports
  • colors.go // Colors are defined differently in backend, cant do much about it i guess
  • colors256.go // ^^
  • dynamic.go // Space key does not exist in tcell, will have to implement
  • flow_layout.go // Just a missing import. added some comments
  • goroutine.go // Issue seems to be due to defaultColor, working on it
  • layout.go
  • mouse.go
  • size.go
  • stdin.go
  • table.go
  • widgets.go

@mjarkk
Copy link
Member Author

mjarkk commented Jan 18, 2020

@MichaelMure I have tried Goland multiple times because it's go integration is better but i dislike so much things about the editor that i just can't get use to it.
My main 2 problems with it is that the interface looks like a airplane cockpit and the startup time is way to long.

I'm trying to use vim more but it's really hard changing the way i think about navigating and changing code after all those year using using the ms word way of navigating / editing code.

@abitrolly
Copy link

Are there any frameworks for testing console programs? The output seems deterministic. Maybe a simple asciinema compare can be used to test and analyse the behavior?

@glvr182
Copy link
Member

glvr182 commented Jan 18, 2020

I have not found anything yet @abitrolly would be useful

@abitrolly
Copy link

Could not find anything specific - an article about autoexpect https://spin.atomicobject.com/2016/01/11/command-line-interface-testing-tools/ and there is also https://github.com/DRMacIver/hecate. Given that, it might be possible to create something like https://github.com/vcr/vcr for terminals.

@gnojus
Copy link

gnojus commented Feb 6, 2020

Hi.
I could help too, if you need any, just not sure what to do.
Test and debug the examples?

@mjarkk
Copy link
Member Author

mjarkk commented Feb 7, 2020

@Nojus297 ,
That would be great.
In the issue body I've placed a list of examples that work and don't working yet.

If you want to help you could checkout on this branch and try to fix one (or more) of the not working examples.

You don't have to worry about me also making progress because i'm dealing with some personal issues that cause me to not enjoy programming lately, hopefully the joy will be back soon and i'll continue working on this.

@gnojus
Copy link

gnojus commented Feb 8, 2020

ok,
my observations so far (without some missing tcell imports)

  • Backspace key doesn't get detected in default editor function, because ch is not 0, in switch statement:

    gocui/edit.go

    Lines 35 to 41 in a34ffb0

    switch {
    case ch != 0 && mod == 0:
    v.EditWrite(ch)
    case key == KeySpace:
    v.EditWrite(' ')
    case key == KeyBackspace || key == KeyBackspace2:
    v.EditDelete(true)
    (affects at least bufs.go)
  • makeWritable function fills some empty cells with zero values. (affects at least table.go and goroutine.go).
    This

    gocui/view.go

    Line 325 in 080bc82

    v.lines[y] = v.lines[y][:newLen]
    should be changed to something like
    v.lines[y] = append(v.lines[y], newCellArray(newLen-len(v.lines[y]))...)
  • Mouse: tcell has different events for mouse and keyboard.
    (https://github.com/gdamore/tcell/blob/bac2bbc5b394b071245b7a56c4ec89d897667564/termbox/compat.go#L296-L301). I guess we can either follow that way, or convert tcell.EventMouse to custom gocui.MouseRight keybinding to keep the same API.
  • Resizing on windows works, but looks to be more laggy (easier to crash size.go by just rapidly resizing). Also more transitional redrawing of the text is visible in the tcell version

@mjarkk
Copy link
Member Author

mjarkk commented Feb 10, 2020

@Nojus297 thanks for looking into this,

makeWritable function fills some empty cells with zero values. (affects at least table.go and goroutine.go).

Thanks so much for finding that, I've looked many hours but couldn't find it.

Mouse: tcell has different events for mouse and keyboard.

I'm not yet completely sure what we should do here, currently i think the best thing to do here is to hold the same gocui API.

Also more transitional redrawing of the text is visible in the tcell version

What do you mean with that?

@abitrolly
Copy link

Mouse: tcell has different events for mouse and keyboard.

Is it possible to avoid mixing keycodes with chars? It is necessary to make shortcuts work regardless of keyboard layout. I don't know if terminal allows that - catch keycode press event and then provide char as a property.

https://github.com/yakshaveinc/go-keycodes - is an attempt to name keycodes as in cross-platform SDL library. This may unify binding definitions and make keyboard configs compatible between platforms and software.

@gnojus
Copy link

gnojus commented Feb 10, 2020

Also more transitional redrawing of the text is visible in the tcell version

What do you mean with that?

Basically, notably more flickering when resizing.
The tcell version: https://we.tl/t-m7kW5BUcF1

@gnojus
Copy link

gnojus commented Feb 10, 2020

@abitrolly what about combined key presses? I'm pretty sure gocui has something like KeyCtrlI, which probably is not defined in the standards (I wouldn't know if it was defined though).

@abitrolly
Copy link

@Nojus297 I found that SDL2 practices for shortcuts are checking which combinations are pressed using a big "if" in an event loop handler. An easier interface is to have a global Keyboard object that maintains the state automatically.

@gnojus
Copy link

gnojus commented Feb 11, 2020

After some looking around, it seems that terminals are very limited in sending keyboard input, but again, I'm not an expert. Different key presses might produce the same result, while not being able to capture other events. Having a standard keycode library doesn't really matter to me, as we have cross-platform and cross-terminal key events by tcell.
See https://github.com/gdamore/tcell/blob/bac2bbc5b394b071245b7a56c4ec89d897667564/key.go#L23-L44

@gnojus
Copy link

gnojus commented Feb 14, 2020

What do you maintainers think?
While I could translate tcell mouse events to current API, it shouldn't probably be me who writes a new API if you decide to go with the other option.

@abitrolly
Copy link

Different key presses might produce the same result, while not being able to capture other events.

This exception need to be raised among terminal developers. Maybe it is time to rethink terminals. The same way https://mosh.org/ did for SSH.

@dankox
Copy link

dankox commented Nov 7, 2020

Hi,
I'm not sure if this PR is still active or not. But I tried to implement tcell/v2 instead of termbox in gocui.
I used the tcell/termbox package at the beginning and then adjust it to better reflect what is used in gocui. I added also TrueColor support (which was my main reason I looked into this, termbox on windows supports only 8 colors) and mouse support.

I tried to implement it in most compatible way I could think of. Colors are starting from 0. I used the original tcell version 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 Attribute(ansicolor+1), then they would have to remove all the minus one from specification.

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 doesn'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 running for me and I added one more to test 24bit colors.
This is my branch in fork:
https://github.com/dankox/gocui/tree/feature/swap-to-tcell

However, I tested it only on remote Linux machine (accessed from Windows Terminal), where everything worked well (even the mouse).
On Windows 10 it works too, but mouse example doesn't work for me. However, mouse example never worked even before, so I'm not sure if it's not because of my terminal and mouse events. 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).

If anybody could try it on other platforms or on the same platform and let me know if there are any problems, that could be helpful.

If you would be interested in including it here, let me know and I can open PR. I didn't try to incorporate the changes right in gocui.go file as I felt that having separate file which wraps the function could simplify any future transfer to other underlying terminal library.

@mjarkk
Copy link
Member Author

mjarkk commented Nov 7, 2020

Holyshit that's awesome.
Please create a PR for that like this one, i think a lot of people would love !!!

Since the creation of this PR (one year ago now) i've had a depression and ever since i lost interest in programming for this project though i still love the project and i would love for it to continue development.
I should have closed this PR already but just hated the idea of ending the development for this, it would be awesome if you want to make this happen.

For testing your branch i'm running Linux and i can test it somewhere in the next few days if you want?

@dankox
Copy link

dankox commented Nov 7, 2020

@mjarkk Great :) I created a PR #73 for it. It is based on the previous PR (regarding frame colors and frame runes), so there might be more changes visible. I hope that's ok.

@mjarkk mjarkk mentioned this pull request Nov 8, 2020
@mjarkk
Copy link
Member Author

mjarkk commented Nov 8, 2020

Closing this in favor of @dankox his pr

@mjarkk mjarkk closed this Nov 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request WIP Work In Progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants