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

Add support for the rectangular area operations #14285

Merged
14 commits merged into from
Nov 10, 2022

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented Oct 23, 2022

Summary of the Pull Request

This PR adds support for the rectangular area escape sequences:
DECCRA, DECFRA, DECERA, DECSERA, DECCARA, DECRARA, and
DECSACE. They provide VT applications with an efficient way to copy,
fill, erase, or change the attributes in a rectangular area of the
screen.

PR Checklist

Detailed Description of the Pull Request / Additional comments

All of these operations take a rectangle, defined by four coordinates.
These need to have defaults applied, potentially need to be clipped
and/or clamped within the active margins, and finally converted to
absolute buffer coordinates. To avoid having to repeat that boilerplate
code everywhere, I've pulled that functionality out into a shared method
which they all use.

With that out of the way, operations like DECFRA (fill), DECERA
(erase), and DECSERA (selective erase) are fairly simple. They're just
filling the given rectangle using the existing methods _FillRect and
_SelectiveEraseRect. DECCRA (copy) is a little more work, because we
didn't have existing code for that in AdaptDispatch, but it's mostly
just cloned from the conhost _CopyRectangle function.

The DECCARA (change attributes) and DECRARA (reverse attributes)
operations are different though. Their coordinates can be interpreted as
either a rectangle, or a stream of character positions (determined by
the DECSACE escape sequence), and they both deal with attribute
manipulation of the target area. So again I've pulled out that common
functionality into some shared methods.

They both also take a list of SGR options which define the attribute
changes that they need to apply to the target area. To parse that data,
I've had to refactor the SGR decoder from the SetGraphicsRendition
method so it could be used with a given TextAttribute instance instead
of just modifying the active attributes.

The way that works in DECCARA, we apply the SGR options to two
TextAttribute instances - one with all rendition bits on, and one with
all off - producing a pair of bit masks. Then by ANDing the target
attributes with the first bit mask, and ORing them with the second, we
can efficiently achieve the same effect as if we'd applied each SGR
option to our target cells one by one.

In the case of DECRARA, we only need to create a single bit mask to
achieve the "reverse attribute" effect. That bit mask is applied to the
target cells with an XOR operation.

Validation Steps Performed

Thanks to @KalleOlaviNiemitalo, we've been able to run a series of tests
on a real VT420, so we have a good idea of how these ops are intended to
work. Our implementation does a reasonably good job of matching that
behavior, but we don't yet support paging, so we don't have the DECCRA
ability to copy between pages, and we also don't have the concept of
"unoccupied" cells, so we can't support that aspect of the streaming
operations.

It's also worth mentioning that the VT420 doesn't have colors, so we
can't be sure exactly how they are meant to interpreted. However, based
on the way the other attribute are handled, and what we know from the
DEC STD 070 documentation, I think it's fair to assume that our handling
of colors is also reasonable.

@ghost ghost added 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 labels Oct 23, 2022
@j4james j4james marked this pull request as ready for review October 26, 2022 00:00
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks!

src/terminal/adapter/adaptDispatch.cpp Outdated Show resolved Hide resolved
Co-authored-by: Carlos Zamora <carlos.zamora@microsoft.com>
Copy link
Member

@lhecker lhecker left a comment

Choose a reason for hiding this comment

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

LGTM, but I did have an idea...

src/terminal/adapter/adaptDispatch.cpp Outdated Show resolved Hide resolved
@lhecker lhecker added the AutoMerge Marked for automatic merge by the bot when requirements are met label Nov 10, 2022
@ghost
Copy link

ghost commented Nov 10, 2022

Hello @lhecker!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 88c3ef6 into microsoft:main Nov 10, 2022
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

Outstanding as always. Thanks James!

@j4james j4james deleted the feature-rect-area branch December 26, 2022 20:57
@ghost
Copy link

ghost commented Jan 24, 2023

🎉Windows Terminal Preview v1.17.1023 has been released which incorporates this pull request.:tada:

Handy links:

zadjii-msft pushed a commit that referenced this pull request Apr 25, 2023
## Summary of the Pull Request

There are certain escape sequences that use the `VTParameters::subspan`
method to access a subsection of the provided parameter list. When the
parameter list is empty, that `subspan` call can end up using an offset
that is out of range, which causes the terminal to crash. This PR stops
that from happening by clamping the offset so it's in range.

## References and Relevant Issues

This bug effected the `DECCARA` and `DECRARA` operations introduced in
PR #14285, and the `DECPS` operation introduced in PR #13208.

## Validation Steps Performed

I've manually confirmed that the sequences mentioned above are no longer
crashing when executed with an empty parameter list, and I've added a
little unit test that checks `VTParameters::subspan` method is returning
the expected results when passed an offset that is out of range.

## PR Checklist
- [x] Closes #15234
- [x] Tests added/passed
- [ ] Documentation updated
- [ ] Schema updated (if necessary)
DHowett pushed a commit that referenced this pull request Apr 25, 2023
## Summary of the Pull Request

There are certain escape sequences that use the `VTParameters::subspan`
method to access a subsection of the provided parameter list. When the
parameter list is empty, that `subspan` call can end up using an offset
that is out of range, which causes the terminal to crash. This PR stops
that from happening by clamping the offset so it's in range.

## References and Relevant Issues

This bug effected the `DECCARA` and `DECRARA` operations introduced in
PR #14285, and the `DECPS` operation introduced in PR #13208.

## Validation Steps Performed

I've manually confirmed that the sequences mentioned above are no longer
crashing when executed with an empty parameter list, and I've added a
little unit test that checks `VTParameters::subspan` method is returning
the expected results when passed an offset that is out of range.

## PR Checklist
- [x] Closes #15234
- [x] Tests added/passed
- [ ] Documentation updated
- [ ] Schema updated (if necessary)

(cherry picked from commit e413a41)
Service-Card-Id: 89001985
Service-Version: 1.17
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-VT Virtual Terminal sequence support AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for the rectangular area operations
5 participants