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

Add line breaks in the debug tap output #13475

Merged
1 commit merged into from
Jul 11, 2022
Merged

Add line breaks in the debug tap output #13475

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/cascadia/TerminalApp/DebugTapConnection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,15 @@ namespace winrt::Microsoft::TerminalApp::implementation

void DebugTapConnection::_OutputHandler(const hstring str)
{
_TerminalOutputHandlers(til::visualize_control_codes(str));
auto output = til::visualize_control_codes(str);
// To make the output easier to read, we introduce a line break whenever
// an LF control is encountered. But at this point, the LF would have
// been converted to U+240A (␊), so that's what we need to search for.
for (size_t lfPos = 0; (lfPos = output.find(L'\u240A', lfPos)) != std::wstring::npos;)
{
output.insert(++lfPos, L"\r\n");
}
Comment on lines +118 to +121
Copy link
Member

@lhecker lhecker Jul 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm fine with this.

Although I'd generally prefer it if til::visualize_control_codes was decomposed, so that we can "recompose" it back into the two behaviors we need (namely newline preserving and not). Something like that:

namespace til
{
    constexpr wchar_t visualize_control_code(wchar_t ch) noexcept
    {
        if (ch < 0x20)
        {
            ch += 0x2400;
        }
        else if (ch == 0x20)
        {
            ch = 0x2423; // replace space with ␣
        }
        else if (ch == 0x7f)
        {
            ch = 0x2421; // replace del with ␡
        }
        return ch;
    }

    _TIL_INLINEPREFIX std::wstring visualize_control_codes(std::wstring str) noexcept
    {
        for (auto& ch : str)
        {
            ch = visualize_control_code(ch);
        }
        return str;
    }

    _TIL_INLINEPREFIX std::wstring visualize_control_codes_with_newlines(std::wstring_view str) noexcept
    {
        std::wstring out;
        out.reserve(str.size());

        wchar_t buffer[3]{ L'\0', L'\r', L'\n' };

        for (const auto& ch : str)
        {
            buffer[0] = visualize_control_code(ch);
            const auto length = buffer[0] != 0x240A ? 1 : 3;
            out.append(&buffer[0], length);
        }

        return out;
    }
}

(I haven't tested, nor commented it, but I think it should work. The main benefit is that it doesn't need to reshuffle the string so that it's only a single O(n) loop.)

Copy link
Member

Choose a reason for hiding this comment

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

I agree, but also... this is an "under duress" debugging feature. It's going to impact performance, even if negligibly, but it is on a low-use surface. I'm cool with it.

I know you probably approved instead of blocking because you're aware of that already. :)

Copy link
Member

Choose a reason for hiding this comment

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

I know you probably approved instead of blocking because you're aware of that already. :)

^ This. 🙂

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry. I guess this was a bit lazy of me, but I didn't want to mess with the existing code, and I figured the performance wasn't that important here.

_TerminalOutputHandlers(output);
}

// Called by the DebugInputTapConnection to print user input
Expand Down