terminal

terminal copied to clipboard

Summary of the Pull Request

Added control in code for not allowed characters in the filename when saving a tab.

References

#13664

PR Checklist

• [ ] Closes #xxx
• [x] CLA signed. If not, go over here and sign the CLA
• [ ] Tests added/passed
• [ ] Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
• [ ] Schema updated.
• [x] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #13664

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Not validated yet. Just draft for now, eventually awaiting feedback.

@zadjii-msft not tried it yet, but something like this could work. Maybe it can be wrapped in a function str_to_filename and put somewhere else.

Side-note: I'm a beginner in C++, so maybe there's a better way of implementing this (maybe using stl algorithms?). Let me know any feedback :)

The STL has no builtin way to efficiently filter a string, so for the (theoretically) best approach you have to build your own algorithm, usually done by using a table approach (pseudo code, might not work):

std::wstring clean_filename(std::wstring str) noexcept {
    static constexpr std::array<bool, 128> filter{{
        // clang-format off
        0 /* NUL */, 0 /* SOH */, 0 /* STX */, 0 /* ETX */, 0 /* EOT */, 0 /* ENQ */, 0 /* ACK */, 0 /* BEL */, 0 /* BS  */, 0 /* HT  */, 0 /* LF  */, 0 /* VT  */, 0 /* FF  */, 0 /* CR  */, 0 /* SO  */, 0 /* SI  */,
        0 /* DLE */, 0 /* DC1 */, 0 /* DC2 */, 0 /* DC3 */, 0 /* DC4 */, 0 /* NAK */, 0 /* SYN */, 0 /* ETB */, 0 /* CAN */, 0 /* EM  */, 0 /* SUB */, 0 /* ESC */, 0 /* FS  */, 0 /* GS  */, 0 /* RS  */, 0 /* US  */,
        0 /* SP  */, 0 /* !   */, 1 /* "   */, 0 /* #   */, 0 /* $   */, 0 /* %   */, 0 /* &   */, 0 /* '   */, 0 /* (   */, 0 /* )   */, 1 /* *   */, 0 /* +   */, 0 /* ,   */, 0 /* -   */, 0 /* .   */, 1 /* /   */,
        0 /* 0   */, 0 /* 1   */, 0 /* 2   */, 0 /* 3   */, 0 /* 4   */, 0 /* 5   */, 0 /* 6   */, 0 /* 7   */, 0 /* 8   */, 0 /* 9   */, 1 /* :   */, 0 /* ;   */, 1 /* <   */, 0 /* =   */, 1 /* >   */, 1 /* ?   */,
        0 /* @   */, 0 /* A   */, 0 /* B   */, 0 /* C   */, 0 /* D   */, 0 /* E   */, 0 /* F   */, 0 /* G   */, 0 /* H   */, 0 /* I   */, 0 /* J   */, 0 /* K   */, 0 /* L   */, 0 /* M   */, 0 /* N   */, 0 /* O   */,
        0 /* P   */, 0 /* Q   */, 0 /* R   */, 0 /* S   */, 0 /* T   */, 0 /* U   */, 0 /* V   */, 0 /* W   */, 0 /* X   */, 0 /* Y   */, 0 /* Z   */, 0 /* [   */, 1 /* \   */, 0 /* ]   */, 0 /* ^   */, 0 /* _   */,
        0 /* `   */, 0 /* a   */, 0 /* b   */, 0 /* c   */, 0 /* d   */, 0 /* e   */, 0 /* f   */, 0 /* g   */, 0 /* h   */, 0 /* i   */, 0 /* j   */, 0 /* k   */, 0 /* l   */, 0 /* m   */, 0 /* n   */, 0 /* o   */,
        0 /* p   */, 0 /* q   */, 0 /* r   */, 0 /* s   */, 0 /* t   */, 0 /* u   */, 0 /* v   */, 0 /* w   */, 0 /* x   */, 0 /* y   */, 0 /* z   */, 0 /* {   */, 1 /* |   */, 0 /* }   */, 0 /* ~   */, 0 /* DEL */,
        // clang-format on
    }};

    std::erase_if(str, [](auto ch) {
        // This lookup is branchless: It always checks the filter, but throws
        // away the result if ch >= 128. This is faster than using `&&` (branchy).
        return filter[ch & 127] & (ch < 128);
    });
    return str;
}

Your approach is fine as well. That code isn't being called particularly often and the string isn't very long.

Wow! Thank you for all your feedback! I think that for now I'm going to correct my code with your suggestions and test it and maybe then improve it using the table. Do you think that this file is a good place to define such a table and function? Or maybe is better to put them in another file? I think that maybe the second option is better since it could be called also by other parts of the code if needed

Do you think that this file is a good place to define such a table and function?

If you choose to use the table based approach, then I'd suggest putting it into our <til/string.h> header: https://github.com/microsoft/terminal/blob/ffe9a0f09bc7b3cb96908d265c93bcd1a4e75fa2/src/inc/til/string.h It already contains similar string-related functions like visualize_control_codes. But I'd approve this PR definitely even without it. Such a function would have no measurable benefit for this use case after all - it would just look nice (which can also be neat in a way 😅). I've adjusted my pseudo-code above to reflect the different code style in that file.

Sorry, i am missing

Thank you again @lhecker ! I'm going to work on this these days

I added the table in <til/string.h> but have not tested it yet since I had some problems with my installation of C++ with VS. I have VS2019 16.11 but when I try to build I get this error:

C:\Program Files (x86)\Microsoft Visual Studio\2019\BuildTools\MSBuild\Microsoft\VC\v160\Microsoft.CppBuild.targets(439,5): error MSB8020: The build tools for v143 (Platform Toolset = 'v143') cannot be found. To build using the v143 build tools, please install v143 build tools. Alternatively, you may upgrade to the current Visual Studio tools by selecting the Project menu or right-click the solution, and then selecting "Retarget solution". [C: \Users\eliaa\Documents\GitHub\terminal\src\tools\closetest\CloseTest.vcxproj]

that searching should be resolved by installing VS2022, or otherwise, the target VS should be changed by the solution properties, is this correct? Anyway, I start to send the code since the installation of VS2022 is taking some time so I can get feedback on the work

Thanks for bearing with us! Yeah, unfortunately as of pretty recently we require VS 2022 😦

Thanks for bearing with us! Yeah, unfortunately as of pretty recently we require VS 2022 😦

~~I'll submit a PR to restore support for VS 2019 soon. We can't officially support it for this project though since only VS 2022 fully supports C++20 unfortunately.~~

Edit: After some deliberation we've decided to not restore support for VS 2019. It just wouldn't work correctly.

Edit: After some deliberation we've decided to not restore support for VS 2019. It just wouldn't work correctly.

If your decision is definitive, consider updating the readme here:

https://github.com/microsoft/terminal/blob/b55bcb50d9f0a8340e3dea9c48834ed09886c790/README.md?plain=1#L295-L296

I can do that little edit in a PR if the project will be supported on VS2022 only from now on

I've opened a PR a few hours ago actually: https://github.com/microsoft/terminal/pull/13709 I'm extremely sorry about that! When I upgraded the project to C++20 and VS 2022 I totally forgot to update the readme. 😥

No, forgive me! I forgot to check it as ready to merge

Hello @DHowett!

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.

Thank you guys! Hope to work with you on other issues in the future!

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

Handy links:

• Release Notes
• Store Download

:tada:Windows Terminal v1.15.2874 has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes
• Store Download