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.

Sign up for GitHub

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

Jump to bottom

Terminal should support ANSI SGR 53 overline attribute #6000

Closed
rbanffy opened this issue May 19, 2020 · 8 comments · Fixed by #6754
Closed

Terminal should support ANSI SGR 53 overline attribute #6000

rbanffy opened this issue May 19, 2020 · 8 comments · Fixed by #6754
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Area-VT Virtual Terminal sequence support Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Console Backlog

Comments

@rbanffy
Copy link

rbanffy commented May 19, 2020

Description of the new feature/enhancement

Most recent terminal applications have extensive support for ANSI terminal commands and attributes. One extremely useful one is the overline (SGR 53) attribute that can be used for status lines and other similar uses.

Windows Terminal on Windows

Gnome Terminal on Ubuntu
@rbanffy rbanffy added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label May 19, 2020
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels May 19, 2020
@zadjii-msft zadjii-msft added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Area-VT Virtual Terminal sequence support Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. labels May 19, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label May 19, 2020
@zadjii-msft zadjii-msft removed the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label May 19, 2020
@zadjii-msft zadjii-msft added this to the Console Backlog milestone May 19, 2020
@zadjii-msft
Copy link
Member

Yep, we probably should.I'm mildly worried that the ExtendedAttributes enum used all the flags in a BYTE, but we'll figure it out.

terminal/src/inc/conattrs.hpp

Lines 11 to 24 in c78f264

enum class ExtendedAttributes : BYTE
{
Normal = 0x00,
Bold = 0x01,
Italics = 0x02,
Blinking = 0x04,
Invisible = 0x08,
CrossedOut = 0x10,
// TODO:GH#2916 add support for these to the parser as well.
Underlined = 0x20, // _technically_ different from LVB_UNDERSCORE, see TODO:GH#2915
DoublyUnderlined = 0x40, // Included for completeness, but not currently supported.
Faint = 0x80, // Included for completeness, but not currently supported.
};
DEFINE_ENUM_FLAG_OPERATORS(ExtendedAttributes);

@DHowett
Copy link
Member

DHowett commented May 19, 2020

So for this one we also have the LVB gridline attributes, technically. We need to figure out how they combine with underline/overline and grid bottom/top

@j4james
Copy link
Collaborator

j4james commented May 19, 2020

So for this one we also have the LVB gridline attributes, technically.

Yeah, I was just going to suggest using the COMMON_LVB_GRID_HORIZONTAL for this as a quick fix. It's no worse than our current usage of COMMON_LVB_UNDERSCORE for underline. I hacked together a quick implementation to test, and it looked OK to me. I'm not exactly sure where the overline is supposed to be rendered, but Mintty at least seems to have it at the very top of the cell. Haven't checked other implementations yet.

As for running out of bits in ExtendedAttributes , note that the lower byte of the _wAttrLegacy word is actually unused as far as I can tell, so we should have an additional 8 bits to play around with.

@DHowett
Copy link
Member

DHowett commented May 19, 2020

Lower byte of ... legacy ...

Holy crap! I never noticed this. That's clever/evil/cl-evil.

@zadjii-msft
Copy link
Member

I was just going to suggest using the COMMON_LVB_GRID_HORIZONTAL for this as a quick fix

That's brilliant and I love it

@DHowett DHowett removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label May 21, 2020
@jdebp jdebp mentioned this issue May 28, 2020
Closed
@j4james
Copy link
Collaborator

j4james commented Jun 25, 2020

FYI, I've got a branch with this implemented, but it's built on top of #6506, so I'll only be able to do a PR once that's merged, which may be a while.

@j4james j4james mentioned this issue Jul 2, 2020
Merged
6 tasks
@ghost ghost added the In-PR This issue has a related PR label Jul 2, 2020
@ghost ghost closed this as completed in #6754 Jul 6, 2020
@ghost ghost removed the In-PR This issue has a related PR label Jul 6, 2020
ghost pushed a commit that referenced this issue Jul 6, 2020
@j4james
Add support for the "overline" graphic rendition attribute (#6754)
70a7ccc 
## Summary of the Pull Request

This PR adds support for the `SGR 53` and `SGR 55` escapes sequences,
which enable and disable the ANSI _overline_ graphic rendition
attribute, the equivalent of the console character attribute
`COMMON_LVB_GRID_HORIZONTAL`. When a character is output with this
attribute set, a horizontal line is rendered at the top of the character
cell.

## PR Checklist
* [x] Closes #6000
* [x] CLA signed. 
* [x] Tests added/passed
* [ ] Documentation updated. 
* [ ] Schema updated.
* [x] I've discussed this with core contributors already.

## Detailed Description of the Pull Request / Additional comments

To start with, I added `SetOverline` and `IsOverlined` methods to the
`TextAttribute` class, to set and get the legacy
`COMMON_LVB_GRID_HORIZONTAL` attribute. Technically there was already an
`IsTopHorizontalDisplayed` method, but I thought it more readable to add
a separate `IsOverlined` as an alias for that.

Then it was just a matter of adding calls to set and reset the attribute
in response to the `SGR 53` and `SGR 55` sequences in the
`SetGraphicsRendition` methods of the two dispatchers. The actual
rendering was already taken care of by the `PaintBufferGridLines` method
in the rendering engines.

The only other change required was to update the `_UpdateExtendedAttrs`
method in the `Xterm256Engine` of the VT renderer, to ensure the
attribute state would be forwarded to the Windows Terminal over conpty.

## Validation Steps Performed

I've extended the existing SGR unit tests to cover the new attribute in
the `AdapterTest`, the `OutputEngineTest`, and the `VtRendererTest`.
I've also manually tested the `SGR 53` and `SGR 55` sequences to confirm
that they do actually render (or remove) an overline on the characters
being output.
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Jul 6, 2020
@jdebp jdebp mentioned this issue Jul 13, 2020
Open
1 task
@ghost
Copy link

ghost commented Jul 22, 2020

🎉This issue was addressed in #6754, which has now been successfully released as Windows Terminal Preview v1.2.2022.0.:tada:

Handy links:

@DHowett
Copy link
Member

DHowett commented Aug 21, 2020

🎉 As of Windows Insider build 20197, this is also supported in the traditional console.

This was referenced May 1, 2023
Closed
Closed
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Area-VT Virtual Terminal sequence support Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Console Backlog
Development

Successfully merging a pull request may close this issue.

Add support for the "overline" graphic rendition attribute j4james/terminal
4 participants
@rbanffy @DHowett @j4james @zadjii-msft