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 Presentation State reports #14998

Merged
merged 13 commits into from
Mar 23, 2023

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented Mar 16, 2023

This PR introduces two new sequences, DECRQPSR and DECRSPS, which
provide a way for applications to query and restore the presentation
state reports. This includes the tab stop report (DECTABSR) and the
cursor information report (DECCIR).

One part of the cursor information report contains the character set
designations and mapped G-sets. But we weren't tracking that data in a
way that could easily be reported, so I needed to do some refactoring in
the TerminalOutput class to make that accessible.

Other than that, the rest was fairly straightforward. It was just a
matter of packaging up all the information into the correct format for
the returned DCS string, and in the case of the restore operations,
parsing the incoming data and applying the new state.

Validation Steps Performed

Thanks to @al20878, we were able to test these operations on a real
VT525, and I've manually verified that our implementation matches that
behavior. I've also added some unit tests covering both reports.

Closes #14984

@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Task It's a feature request, but it doesn't really need a major design. Area-VT Virtual Terminal sequence support labels Mar 16, 2023
@j4james
Copy link
Collaborator Author

j4james commented Mar 16, 2023

I think the remaining audit failures are an existing issue - you'll see the same errors in a number of recent PRs. I've submitted a fix in PR #15002.

@j4james j4james marked this pull request as ready for review March 16, 2023 11:28
src/terminal/parser/OutputStateMachineEngine.hpp Outdated Show resolved Hide resolved
ITermDispatch::StringHandler AdaptDispatch::_RestoreCursorInformation()
{
// clang-format off
enum Field { Row, Column, Page, SGR, Attr, Flags, GL, GR, Sizes, G0, G1, G2, G3 };
Copy link
Member

Choose a reason for hiding this comment

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

This is clever (this whole function, despite me marking just one line) and I bet @lhecker has thoughts about it 😁

Copy link
Member

Choose a reason for hiding this comment

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

It's pretty much a functor that has been degranulated into a lambda with captured state!

Copy link
Member

Choose a reason for hiding this comment

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

I haven't considered writing a state machine this way yet. This is pretty neat technique!

src/terminal/adapter/terminalOutput.cpp Outdated Show resolved Hide resolved
src/terminal/adapter/DispatchTypes.hpp Outdated Show resolved Hide resolved
Comment on lines 60 to 69
inline std::wostream& operator<<(std::wostream& out, const VTID& id)
{
uint64_t value = id;
do
{
out << gsl::narrow_cast<char>(value & 0xFF);
value >>= CHAR_BIT;
} while (value);
return out;
}
Copy link
Member

@lhecker lhecker Mar 22, 2023

Choose a reason for hiding this comment

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

It'd be possible to turn VTID into a:

union VTID
{
private:
    uint64_t _value = 0;
    char _string[8];
};

and it'd be entirely valid C++. That way we could get the string out of it in constant time and simplify both string related functions to a simple std::copy_n for FromString and return &_string[0] for the ToString function. All we need to do to make this work is to ensure that _string[7] is 0 in the 2 constructors this class has, which I feel like is a feasible modification to do (i.e. via _value = value & 0x00FFFFFFFFFFFFFF).

Copy link
Member

Choose a reason for hiding this comment

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

I realized that this won't work because constexpr doesn't support reinterpret_cast or equivalents (like unions). So here's another alternative:

class VTID
{
public:
    template<size_t Length>
    consteval VTID(const char (&s)[Length]) :
        _value{ _FromString(s) }
    {
    }

    constexpr VTID(const uint64_t value) :
        // Ensure we got a trailing 0 for our string conversion.
        _value{ value & 0x00FFFFFFFFFFFFFF }
    {
    }

private:
    template<size_t Length>
    static consteval uint64_t _FromString(const char (&s)[Length])
    {
        // Length already includes the trailing 0 of the string literal.
        static_assert(Length <= sizeof(_value));

        char buffer[sizeof(_value)]{};
        std::copy_n(&s[0], Length, &buffer[0]);

        return std::bit_cast<uint64_t>(buffer);
    }

    uint64_t _value;
};

I should mention that this is just an idea and doesn't necessarily mean that I'm convinced it's a good one. 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I think this is good idea. I'll give it a try.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the end I just used your union trick to get a more efficient ToString and left the rest more or less as it was (other than making sure there was always a null terminator).

I had initially thought I could make the ToString return a char*, but the fmt::format call couldn't cope with that (I'm assuming because of the mismatch between char and wchar_t). It was happy with a string_view though.

It's maybe not perfect, but I think this is an improvement on the initial implementation.

src/terminal/adapter/adaptDispatch.cpp Outdated Show resolved Hide resolved
src/terminal/adapter/adaptDispatch.cpp Outdated Show resolved Hide resolved
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.

Reading through my comments again, I noticed they're all just minor nits. Please feel free to address them, but I'm also good with the PR as is. 🙂

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.

I love the new VTID.

@DHowett DHowett merged commit 0f7d1f4 into microsoft:main Mar 23, 2023
@j4james j4james deleted the feature-decrqpsr branch March 28, 2023 13:43
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 Issue-Task It's a feature request, but it doesn't really need a major design.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Presentation State reports
3 participants