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

Upgrade tcell to v2.8.0 and add support for Underline style and color #680

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

noborus
Copy link
Owner

@noborus noborus commented Jan 10, 2025

  • Upgraded tcell to version v2.8.0.
  • Added support for Underline style and color in ov.
  • Modified oviewer/convert_es.go to handle the new Underline styles and colors.
  • Updated oviewer/ovstyle.go to apply the new Underline styles and colors.
  • Upgraded other modules.

- Upgraded tcell to version v2.8.0.
- Added support for Underline style and color in ov.
- Modified oviewer/convert_es.go to handle the new Underline styles and colors.
- Updated oviewer/ovstyle.go to apply the new Underline styles and colors.
- Upgraded other modules.
@noborus noborus marked this pull request as ready for review January 10, 2025 03:15
Comment on lines 150 to 153
return 0
}
if tcell.UnderlineStyle(n) > tcell.UnderlineStyleDashed {
return 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there no constants for this 0 ?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, there is. I'll fix it, thanks.

Comment on lines +144 to +146
// underLineStyle sets the underline style.
// only support 0-5.
// 0: None, 1: Single, 2: Double, 3: Curly, 4: Dotted, 5: Dashed
Copy link
Contributor

Choose a reason for hiding this comment

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

About this

#680 (comment)

Maybe you could link the constants here with a godoc link

Something like this (pseudo code)

Suggested change
// underLineStyle sets the underline style.
// only support 0-5.
// 0: None, 1: Single, 2: Double, 3: Curly, 4: Dotted, 5: Dashed
// underLineStyle sets the underline style.
// only support 0-5.
// 0: [tview.WhateverNone], 1: [tview.WhateverSingle], 2: Double, 3: Curly, 4: Dotted, 5: Dashed

if err != nil {
return 0
}
if tcell.UnderlineStyle(n) > tcell.UnderlineStyleDashed {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks strange

I assume you only wanto support 0-5 range, but these are iota, I assume. So comparing them is a but strange

I would use a switch with the exact value you support in one case.

Also, if you only supports the value "0", "1", maybe you could avoid the string to int conversion

Copy link
Contributor

@ccoVeille ccoVeille Jan 10, 2025

Choose a reason for hiding this comment

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

I saw you added negative checks now

But what do you think about my comment just above this one?

EDIT @nobe4 wrong ping 😬😅
@noburus

Copy link
Owner Author

Choose a reason for hiding this comment

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

This is how to do the behavior of an unauthorized value outside the specifications.
The operation changes with Terminal-Emulator, so it may change in the future.

Originally, it is derived from the escape sequence specification, so it doesn't matter much whether it is an iota.

noburus is not a person concerned. 😥

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, a double wrong ping 🤦‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants