Disallow certain non-filename characters in Export Text #13693

Eschivo · 2022-08-05T21:29:44Z

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

Eschivo · 2022-08-05T21:33:59Z

@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 :)

src/cascadia/TerminalApp/TabManagement.cpp

lhecker · 2022-08-05T22:22:45Z

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.

Eschivo · 2022-08-06T06:38:49Z

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

lhecker · 2022-08-07T01:29:02Z

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.

xtremevipper · 2022-08-07T13:11:31Z

Sorry, i am missing

Eschivo · 2022-08-08T12:07:54Z

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

Eschivo · 2022-08-09T16:55:59Z

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

DHowett · 2022-08-09T17:09:47Z

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

lhecker · 2022-08-09T17:47:46Z

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.

src/inc/til/string.h

Co-authored-by: Leonard Hecker <leonard@hecker.io>

Eschivo · 2022-08-09T19:23:01Z

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:

terminal/README.md

Lines 295 to 296 in b55bcb5

    
           * You must have at least [VS 
        
             2019](https://visualstudio.microsoft.com/downloads/) installed

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

lhecker · 2022-08-09T20:11:52Z

I've opened a PR a few hours ago actually: #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. 😥

zadjii-msft

Any reason this is still in draft? This looks good to me!

Eschivo · 2022-08-11T16:33:55Z

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

DHowett

This is excellent work, thank you for doing it!

DHowett · 2022-08-11T20:56:04Z

src/inc/til/string.h

+
+        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).


this SCREAMS "leonard code". I love it.

ghost · 2022-08-11T20:56:58Z

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.

Eschivo · 2022-08-14T12:40:10Z

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

Added control in code for not allowed characters in the filename when saving a tab. Closes #13664 (cherry picked from commit 8721573) Service-Card-Id: 85487287 Service-Version: 1.15

…)" This reverts commit 64c774e.

ghost · 2022-09-13T17:04:19Z

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

Handy links:

Added control in code for not allowed characters in the filename when saving a tab. Closes #13664 (cherry picked from commit 8721573) Service-Card-Id: 85487287 Service-Version: 1.15

ghost · 2022-10-18T21:50:19Z

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

Handy links:

Added control in code for not allowed characters in filename

a860d96

lhecker reviewed Aug 5, 2022

View reviewed changes

src/cascadia/TerminalApp/TabManagement.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/TabManagement.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/TabManagement.cpp Outdated Show resolved Hide resolved

Filename correction using lookup table

4184ce6

lhecker approved these changes Aug 9, 2022

View reviewed changes

src/inc/til/string.h Outdated Show resolved Hide resolved

Correction of return in erase_if inside clean_filename

3d3bb13

Co-authored-by: Leonard Hecker <leonard@hecker.io>

zadjii-msft approved these changes Aug 11, 2022

View reviewed changes

zadjii-msft linked an issue Aug 11, 2022 that may be closed by this pull request

Export Text when in Admin mode filename suggestion has invalid character ":" #13664

Closed

zadjii-msft marked this pull request as ready for review August 11, 2022 16:54

ghost added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Aug 11, 2022

DHowett approved these changes Aug 11, 2022

View reviewed changes

DHowett changed the title ~~Added control in code for not allowed characters in filename~~ Disallow certain non-filename characters in Export Text Aug 11, 2022

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 11, 2022

ghost merged commit 8721573 into microsoft:main Aug 11, 2022

Eschivo deleted the filename_correction_tabsave branch August 14, 2022 12:40

DHowett added a commit that referenced this pull request Sep 9, 2022

Revert "Disallow certain non-filename characters in Export Text (#13693…

06648e3

…)" This reverts commit 64c774e.

ghost mentioned this pull request Sep 13, 2022

Export Text when in Admin mode filename suggestion has invalid character ":" #13664

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disallow certain non-filename characters in Export Text #13693

Disallow certain non-filename characters in Export Text #13693

Eschivo commented Aug 5, 2022 •

edited by DHowett

Loading

Eschivo commented Aug 5, 2022

lhecker commented Aug 5, 2022 •

edited

Loading

Eschivo commented Aug 6, 2022

lhecker commented Aug 7, 2022

xtremevipper commented Aug 7, 2022

Eschivo commented Aug 8, 2022

Eschivo commented Aug 9, 2022 •

edited

Loading

DHowett commented Aug 9, 2022

lhecker commented Aug 9, 2022 •

edited

Loading

Eschivo commented Aug 9, 2022

lhecker commented Aug 9, 2022 •

edited

Loading

zadjii-msft left a comment

Eschivo commented Aug 11, 2022 via email •

edited by ghost

Loading

DHowett left a comment

DHowett Aug 11, 2022

ghost commented Aug 11, 2022

Eschivo commented Aug 14, 2022

ghost commented Sep 13, 2022

ghost commented Oct 18, 2022

Disallow certain non-filename characters in Export Text #13693

Disallow certain non-filename characters in Export Text #13693

Conversation

Eschivo commented Aug 5, 2022 • edited by DHowett Loading

Eschivo commented Aug 5, 2022

lhecker commented Aug 5, 2022 • edited Loading

Eschivo commented Aug 6, 2022

lhecker commented Aug 7, 2022

xtremevipper commented Aug 7, 2022

Eschivo commented Aug 8, 2022

Eschivo commented Aug 9, 2022 • edited Loading

DHowett commented Aug 9, 2022

lhecker commented Aug 9, 2022 • edited Loading

Eschivo commented Aug 9, 2022

lhecker commented Aug 9, 2022 • edited Loading

zadjii-msft left a comment

Choose a reason for hiding this comment

Eschivo commented Aug 11, 2022 via email • edited by ghost Loading

DHowett left a comment

Choose a reason for hiding this comment

DHowett Aug 11, 2022

Choose a reason for hiding this comment

ghost commented Aug 11, 2022

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.

Eschivo commented Aug 14, 2022

ghost commented Sep 13, 2022

ghost commented Oct 18, 2022

Eschivo commented Aug 5, 2022 •

edited by DHowett

Loading

lhecker commented Aug 5, 2022 •

edited

Loading

Eschivo commented Aug 9, 2022 •

edited

Loading

lhecker commented Aug 9, 2022 •

edited

Loading

lhecker commented Aug 9, 2022 •

edited

Loading

Eschivo commented Aug 11, 2022 via email •

edited by ghost

Loading

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.