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

Merge the legacy and extended attributes #14031

Closed
j4james opened this issue Sep 18, 2022 · 4 comments · Fixed by #14036
Closed

Merge the legacy and extended attributes #14031

j4james opened this issue Sep 18, 2022 · 4 comments · Fixed by #14036
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@j4james
Copy link
Collaborator

j4james commented Sep 18, 2022

Description of the new feature/enhancement

In order to implement #14029, there was a need to add another bit in the ExtendedAttributes enum, and it occurred to me while looking at that code that it might be a good time to do something refactoring there first.

At the moment we've got two attribute fields in the TextAttribute class: _wAttrLegacy, which only uses 5 of its available 16 bits; and _extendedAttrs, which uses 8 of its available 16 bits. If we merged the two together into a single 16-bit field, we'd still have 3 bits to spare, and the TextAttribute class would be two bytes smaller.

Proposed technical implementation details (optional)

The idea would be to drop the legacy attributes, and replace the existing ExtendedAttributes enum with something like this:

enum class CharacterAttributes : uint16_t
{
    Normal = 0x00,
    Intense = 0x01,
    Italics = 0x02,
    Blinking = 0x04,
    Invisible = 0x08,
    CrossedOut = 0x10,
    Underlined = 0x20,
    DoublyUnderlined = 0x40,
    Faint = 0x80,
    Unused1 = 0x100,
    Unused2 = 0x200,
    TopGridline = COMMON_LVB_GRID_HORIZONTAL, // 0x400
    LeftGridline = COMMON_LVB_GRID_LVERTICAL, // 0x800
    RightGridline = COMMON_LVB_GRID_RVERTICAL, // 0x1000
    Unused3 = 0x2000,
    ReverseVideo = COMMON_LVB_REVERSE_VIDEO, // 0x4000
    BottomGridline = COMMON_LVB_UNDERSCORE // 0x8000
};

The added legacy bits are in the exact same position they were in the original field, so when initializing the TextAttribute class from a legacy value, we just have to mask out the relevant bits and write them directly to this field. Same approach works when converting from a TextAttribute back to a legacy value.

What do you think? Can I put together a PR with this change?
@j4james j4james added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Sep 18, 2022
@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 Sep 18, 2022
@lhecker
Copy link
Member

lhecker commented Sep 18, 2022

I've considered merging them as well in the past, but simply haven't had time for it yet. I like it. 👍

@DHowett
Copy link
Member

DHowett commented Sep 19, 2022

This shouldn't impact our ability to expand it to 32 bits later, right?

As long as none of the legacy API operations are impacted (either by gaining new unexpected abilities or by losing them,) I'm totally cool with it. Thanks for proposing!

@j4james j4james mentioned this issue Sep 19, 2022
Merged
@ghost ghost added the In-PR This issue has a related PR label Sep 19, 2022
@j4james
Copy link
Collaborator Author

j4james commented Sep 19, 2022

This shouldn't impact our ability to expand it to 32 bits later, right?

I don't think so. We may have to reorder the fields to get the alignment right, but that shouldn't be an issue.

As long as none of the legacy API operations are impacted

Yeah, they shouldn't be. The code which handles the conversion to and from legacy attributes is almost identical to what it was before.

@lhecker lhecker added Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Issue-Task It's a feature request, but it doesn't really need a major design. Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. and removed Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. labels Sep 20, 2022
@ghost ghost closed this as completed in #14036 Sep 20, 2022
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Sep 20, 2022
ghost pushed a commit that referenced this issue Sep 20, 2022
@j4james
Merge the legacy and extended attributes (#14036)
1ce22a8 
This PR attempts to simplify the `TextAttribute` class by merging the
two fields that were previously storing the "legacy" attributes and the
"extended" attributes separately.

When the `TextAttribute` class is initialized with a legacy value, we
were masking off the `META_ATTRS` bits to store in the `_wAttrLegacy`
field, and then additionally clearing the `COMMON_LVB_SBCSDBCS` bits,
so there were only 5 bits that were actually used in the end. We also
had an additional `_extendedAttrs` field holding other VT attributes,
which only used 8 of its available 16 bits.

In this PR I've now merged the the two sets of attributes into one enum,
so they all fit in a single 16-bit value. The legacy attributes retain
the same bit positions they originally had, so we can mask them off from
an incoming legacy value as we did before. I've just simplified the
process somewhat by creating a `USED_META_ATTRS` mask that covers the
exact subset of meta attributes that we care about.

The new enum that holds the combined attributes has now been named
`CharacterAttributes` rather than `ExtendedAttributes`, since that seems
to be the term typically used in VT documentation. This covers both
rendition/visual attributes and logical attributes (not yet used, but we
will need them at some point to support selective erase operations).

While making these changes I also noticed the `IsLeadingByte` and
`IsTrailingByte` methods weren't actually used anywhere, and weren't
correctly implemented anyway, so I've removed those now.

## Validation Steps Performed

I've manually run a number of attribute test scripts which cover both
legacy and VT operations, and everything still appears to be working
correctly.

Closes #14031
@ghost
Copy link

ghost commented Jan 24, 2023

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

Handy links:

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-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Merge the legacy and extended attributes j4james/terminal
3 participants
@DHowett @lhecker @j4james