mirror of
https://github.com/mum4k/termdash.git
synced 2026-04-27 03:15:55 +03:00
[GH-ISSUE #223] Does not render on Windows Terminal (preview) #123
Labels
No labels
bug
cleanup
enhancement
enhancement
enhancement
good first issue
help wanted
help wanted
pull-request
question
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
starred/termdash#123
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @kvnxiao on GitHub (Feb 12, 2020).
Original GitHub issue: https://github.com/mum4k/termdash/issues/223
Originally assigned to: @kvnxiao on GitHub.
Currently using https://github.com/microsoft/terminal from the window's app store (which is still in preview since it has not officially released yet) on my Windows machine.
I'm trying to make a cross-platform terminal application but it doesn't seem to work at all through Windows Terminal, regardless of the underlying shell I use (e.g. neither cmd nor powershell work when launched through Windows Terminal).
I am aware that termbox-go does not work through mintty or cygwin, but I can confirm that termbox-go and termui do work through the new Windows Terminal application.
Here is an example of a termui application running in windows terminal:
However when I try to build the TextInputDemo as an executable and run this through Windows Terminal, it does not show up at all.
It simply shows a flashing cursor at the top left.
@mum4k commented on GitHub (Feb 12, 2020):
Thank you for the bug report. I currently have no easy way to test this on windows so hoping to get some help if you have the time.
Good to hear that termbox works in the terminal. Termdash completely depends on termbox-go when it comes to rendering, so it might just be the case of some missing / different initialization of termbox. Would you be able to experiment, find the differences between how termdash and termui initialize termbox? That is where the answer might be. Once we have the differences, we can easily modify the code to fix this.
@kvnxiao commented on GitHub (Feb 12, 2020):
I can look into it sometime this week.
I have also noticed that the text demo here: https://github.com/mum4k/termdash/tree/master/widgets/text/textdemo
simply does not work on Windows, even when launched from conhost through cmd or powershell (and not Windows Terminal).
The text is mangled, and the rotating / scrolling lines of text does not show up properly.
I'll get a screenshot in a bit and post it here for tracking.
P.S. I really like the way termdash is written from an API standpoint, so I would definitely like to see it work on Windows
@mum4k commented on GitHub (Feb 12, 2020):
That is interesting, do the other demos work or none of them work? I do seem to recall that there was some issue on windows around setting the right font (one that supports the unicode characters that are used to draw borders, etc). It might be unrelated though, there was some relevant discussion here: https://github.com/mum4k/termdash/issues/99
Looking forward to hear from you after you experiment with it and thanks.
@kvnxiao commented on GitHub (Feb 13, 2020):
Kind of unrelated to the main issue
FYI, here is what the textdemo one looks like running through regular conhost (directly opened through the built .exe)
Seems like using non-english languages like having the Chinese characters screw up the offsets a bit. I blame Windows for this.
Removing the Chinese characters seems to prevent the issue from popping up, and the scrolling text shows up properly now:
A bit of a shame but it might mean that the Windows conhost can't properly support monospaced CJK fonts to take up only 1 column space on screen for whatever reason.
In summary: all the widgets work on Windows when ran directly or through cmd/powershell, which uses the default conhost.exe, but NOT Windows Terminal, which is the main issue here
@kvnxiao commented on GitHub (Feb 13, 2020):
For Windows Terminal:
So it seems like there is an issue with whenever an empty space exists between the text and the next element (whether the next element is another container, or the border of the current container).
If you could point me to the right directly as to how exactly the code is rendering the different widgets at different locations within the terminal view? (e.g. how is each line created)
I'm uploading a gif as well to show that some lines do end up showing up as long as text needs to be updated at the current row (in this case it is from the scrolling text widget).
@kvnxiao commented on GitHub (Feb 13, 2020):
After further exploring,
I found that the culprit was termbox-go itself with how it renders. (maybe?)
I tested using tcell and it's termbox api compatibility here https://github.com/gdamore/tcell/blob/master/termbox/compat.go
and guess what? It works!
It even works with the Chinese text.
It also works with the textinputdemo which was the one I originally tried to run as shown in the first comment:
The only thing with the tcell termbox-compatibility is that the input events do not work properly, but this was obviously a quick replacement test to see if it would fix the rendering.
At this point I'm not sure whether I should be blaming termbox-go's way of rendering or Windows Terminal for still being a preview (maybe the 1.0.0 release of Windows Terminal will fix how termbox-go renders?).
My recommendation would be to directly pursue https://github.com/mum4k/termdash/issues/100 so that different terminal rendering backends can be used, seeing as this appears to render fine on Windows when using tcell.
This would obviously entail more than just using the termbox compat file from tcell, since there are certain issues with the compat api:
@mum4k commented on GitHub (Feb 13, 2020):
This was really helpful investigation, thank you @kvnxiao.
I agree that pursuing #100 sounds like the right option. When I started developing Termdash, I had a replacement of termbox in mind. All the terminal interactions happen through the terminalapi layer:
https://github.com/mum4k/termdash/tree/master/terminal/terminalapi
Where termbox-go is just one implementation of that api:
https://github.com/mum4k/termdash/tree/master/terminal/termbox
I think the right way forward would be to add a tcell implementation of the same API. That is where we can handle any necessary translations of keyboard events, mouse events, etc.
Is this something you would be interested in working on?
@kvnxiao commented on GitHub (Feb 13, 2020):
This is something I can definitely look into trying out on my own branch.
@mum4k commented on GitHub (Feb 14, 2020):
Great, please feel free to reach out with any questions or concerns and thank you for your help!
@kvnxiao commented on GitHub (Feb 15, 2020):
I've created a few commits to my forked repo with a new tcell backend that implements the terminalapi interface.
https://github.com/mum4k/termdash/compare/master...kvnxiao:master
This includes proper colour setting, keyboard, and mouse events.
There are a few caveats (differences from termbox-go) I'd like to discuss before moving forward:
I think moving forward, maybe it would be cool to support multi-mouse buttons from the terminalapi side as well, but I am not sure how termbox handles that.
And before I open the PR, is there anything I need to be weary of? For example, what do I do about license headers?
@mum4k commented on GitHub (Feb 15, 2020):
That looks really good, glad to hear it worked out. Thank you for raising the two discussion points. In general - we don't have to limit ourselves to the functionality provided by termbox-go, i.e. it would be ok to even modify terminalapi as long as it remains backward compatible with termbox-go and with the existing functionality utilised by termdash. With that said, I don't think we should attempt to do that in a single PR as it might bloat the scope. We can add tcell now with the same functionality as termbox-go and expand on it later if desired.
github.com/mum4k/termdash@68bf0566e6/terminal/termbox/termbox.go (L45-L51)If yes, the way I see it - this is an optional argument specific to termbox-go, it isn't declared in terminalapi and doesn't have to be supported by the tcell implementation assuming everything works without it.
If we choose to work on multiple mouse buttons in the future, we can modify terminalapi and indicate that this is only supported with the tcell terminal and not termbox. We could come up with some form of a capability registration. So that individual terminal implementation register which parts of the events they are capable off. That way we could return an error if the user creates termbox-go but expects to get multiple mouse button events somewhere down the stack. Or we could decide to completely abandon termbox-go in the future.
Another interesting thing we should look at is performance. I wonder what is the performance impact of switching from termbox-go to termdash CPU and Memory wise. Have you thought about running termdashdemo with pprof using the two terminal implementations and compare (https://blog.golang.org/profiling-go-programs)? Of course this doesn't have to be done for the current PR, maybe we can open a separate issue to track this effort.
As for other things to be aware off, the automatic scripts that run when you create the PR will inform you about most of these:
-- https://github.com/mum4k/termdash/wiki/Terminal-API
-- https://github.com/mum4k/termdash/wiki/Termbox-API
-- and we should probably create a new one for tcell
Hope this helps, please let me know if you have any other questions and thanks again for improving termdash.
@kvnxiao commented on GitHub (Feb 16, 2020):
Do you think it would be a good idea to have some sort of build flag to choose the implementation when building a project that uses termdash, so that the other implementation simply isn’t included as a dependency, thus resulting in smaller file sizes? (Not exactly sure how I would go about doing this)
I will work on the rest of the changes for the PR sometime after next week as I’ve just gotten back home for a week’s worth of break.
@mum4k commented on GitHub (Feb 16, 2020):
That is a good point, but I think that may already work as desired Considering that termbox and tcell are separate packages, user building termdash imports one of them, initializes the chosen terminal implementation and passes that into the container instance, e.g.:
github.com/mum4k/termdash@68bf0566e6/termdashdemo/termdashdemo.go (L470-L479)So if the user only imports the tcell package and doesn't import the termbox package, the termbox package and any of its dependencies won't get linked or included in the build. This of course assumes that we only import termbox-go inside the termbox package. Please let me know if I am missing anything.
Enjoy your break at home and talk to you afterwards. We have a long weekend here so I too am taking it easy.
@kvnxiao commented on GitHub (Feb 29, 2020):
Put up the PR, probably a few things will still need to be changed before it looks good to merge, let me know!