mirror of
https://github.com/mum4k/termdash.git
synced 2026-04-27 03:15:55 +03:00
[GH-ISSUE #254] Update tcell to the latest 2.x version #139
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#139
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 @mum4k on GitHub (Nov 13, 2020).
Original GitHub issue: https://github.com/mum4k/termdash/issues/254
Originally assigned to: @dyc3 on GitHub.
Changelog: https://github.com/gdamore/tcell/blob/master/CHANGESv2.adoc
And while we are at it, we can also consider making
tcellthe default in all code examples.@mum4k commented on GitHub (Nov 14, 2020):
@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:
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 commented on GitHub (Nov 14, 2020):
@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 commented on GitHub (Nov 14, 2020):
I'll take a look at it tomorrow. It's currently 3 am where I am and I need sleep haha
@mum4k commented on GitHub (Nov 14, 2020):
We seem to be in the same timezone, so I can relate...
And thank you of course.
@dyc3 commented on GitHub (Nov 14, 2020):
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 commented on GitHub (Nov 14, 2020):
While that is good news, it is likely that it only gives a false sense of security. The unit tests in
termdashonly 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
termboxortcell, so the only way to tell if this works correctly may be by running the demos and exercising the functionality.@dyc3 commented on GitHub (Nov 14, 2020):
Color is broken because the number system was changed. Font modifiers still work though.
colors.godiff of 1.4 and 2.0:@mum4k commented on GitHub (Nov 14, 2020):
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 commented on GitHub (Nov 14, 2020):
I'm kinda stumped.
I think
fixColorneeds to addtcell.ColorValidto the color value? When I try that, I get colors back, but they are all wrong.And the tests for
cellColorandfixColorare still passing, which doesn't really make sense because I'm changing the behavior of both of those functions...@mum4k commented on GitHub (Nov 14, 2020):
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 commented on GitHub (Nov 15, 2020):
I think I understand the problem now and I am fairly convinced this is a bug in our
tcelllibrary. I will send you a PR branched off your branch that will fix that and also align our color definitions with the standardtcellis using. Since that is the official standard. I just need to figure out how to make this backward compatible for thetermboxusers out there.@mum4k commented on GitHub (Nov 15, 2020):
The following PR is addressing the color problem: https://github.com/dyc3/termdash/pull/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 commented on GitHub (Nov 15, 2020):
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 commented on GitHub (Nov 15, 2020):
Take your time and feel free to let me know if you would like me to take point on this.
@dyc3 commented on GitHub (Nov 15, 2020):
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 commented on GitHub (Nov 15, 2020):
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:github.com/mum4k/termdash@2a7dafa3a8/keyboard/keyboard.go (L59)Or do you mean that the new support for modifiers added by
tcellin v2 isn't reflected in our API?github.com/gdamore/tcell@7d87d8188c/key.go (L73-L79)In other words - did any existing functionality change after upgrading
tcellto v2 or are we just missing new functionality?Additionally is #260 ready for review or do we still need to add something?
@dyc3 commented on GitHub (Nov 15, 2020):
Oh wait, I see.
The keys like
KeyCtrlAwork, but Shift+F1 givesKeyF1.We're just missing new functionality. #260 is ready to review
@mum4k commented on GitHub (Nov 15, 2020):
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 commented on GitHub (Nov 18, 2020):
Resolved by #266.