Add reserved name handling and add user filename validation#42
Conversation
|
If you decide to accept and merge this, could you bump the submodule reference in s25client? There's a follow-up PR there that depends on this. Thanks! |
Flamefire
left a comment
There was a problem hiding this comment.
For user-provided filenames - such as save games and the incoming addon presets feature (Return-To-The-Roots/s25client#1951) - the intended approach is different: invalid characters will be suppressed at the ctrlEdit input level using isValidFileNameChar, and isValidFileName will perform a final structural check on save. If validation fails, the user receives a general message asking them to correct the name before saving.
I'm not sure about the suppressing part.
I think the easiest way would be to simply run the users input through makePortableName and show an error if the result is different.
The current handling seems complicated: isValidFileName allows invalid characters and isValidFileNameChar seemingly allows at least non-portable characters.
On second thought I think it does make sense to allow users more freedom in their choice of file names, so fine by me.
Just fix isValidFileName to use isValidFileNameChar and add a single test for that (or 3: invalid char at start, middle, end, but I'd say one is enough)
I'm not a big fan of verbose tests, but having the valid and invalid chars in an array and using a loop might not be much clearer either. Just mentioning it for consideration where it might make sense in the future.
Co-authored-by: Alexander Grund <Flamefire@users.noreply.github.com>
There was a problem hiding this comment.
I added isValidFileNameChar to isValidFileName, applied your other suggestions and trimmed the test cases. Will remember for the future to be a bit more conservative with them :-)
With the suppressing in ctrlEdit (similarly to numberOnly_ but for this it would be fileNameOnly_) I found it the easiest solution to implement without passing back some error codes and creating text messages for each case...
Would you like ctrlEdit modifications (literally 9 new lines including comments, and 4 removed for double-space bug fix) and save game fixes in a separate PR, single PR or added to addon-presets PR? Personally would prefer addon-presets, then I wouldn't have to wait for the ctrlEdit PR to be merged but that is impatient me ;-)
Flamefire
left a comment
There was a problem hiding this comment.
trimmed the test cases. Will remember for the future to be a bit more conservative with them :-)
A bit too much, I might have been not clear enough. See suggestions. For each test simply think if this could potentially catch a (class of) mistake. I put my own reasoning on the reserved-with-extension suggestion. The others are similar: CON-NUL-nul-Lpt --> 2 examples, 3rd is same as 2 in lowercase, and a 4th mixed-case one.
For the special chars I was aiming for reasonably common ones.
Would you like ctrlEdit modifications (literally 9 new lines including comments, and 4 removed for double-space bug fix) and save game fixes in a separate PR, single PR or added to addon-presets PR? Personally would prefer addon-presets, then I wouldn't have to wait for the ctrlEdit PR to be merged but that is impatient me ;-)
Fine for me to add it where it's used, ie. the existing PR
Co-authored-by: Alexander Grund <Flamefire@users.noreply.github.com>
… string_view find
…inux ext4 and Windows NTFS
Summary
Adds Windows reserved device name support to
makePortableNameand introduces two new predicates (isValidFileNameChar,isValidFileName) for validating user-provided filenames.Motivation
These utilities are needed in s25client to validate user-provided filenames in save file dialogs and similar text input controls, where both real-time character filtering and save-time structural validation are required.
Note
makePortableFileName(and by extensionmakePortableName) is intended for programmatic filename creation - it silently and automatically replaces invalid characters without user involvement.For user-provided filenames - such as save games and the incoming addon presets feature (s25client PR #1951) - the intended approach is different: invalid characters will be suppressed at the
ctrlEditinput level usingisValidFileNameChar, andisValidFileNamewill perform a final structural check on save. If validation fails, the user receives a general message asking them to correct the name before saving. There is no automatic character replacement and no per-reason error detail - both would complicate the implementation, and silent replacement in particular would create an inconsistent experience where the saved filename silently differs from what the user typed.Changes
makePortableNameWindows reserved device names (
CON,PRN,AUX,NUL,COM0–COM9,LPT0–LPT9) were previously passed through unchanged when they already satisfiedbfs::portable_name. This caused a subtle bug where e.g."nul"was returned as-is despite being unusable as a filename on Windows. The fix appends'_'to any reserved name.isValidFileNameChar(char32_t)New predicate for real-time filtering of user-typed filename characters. Rejects control characters and characters forbidden on Windows (
< > : " / \ | ? *), which is the most restrictive set across all supported platforms.isValidFileName(const std::string&)New structural validator for complete user-provided filenames. Rejects:
fileNameis checked against the segment before the first dot, so "nul.ini" is also rejectedComment corrections
The existing doc comments on
makePortableName,makePortableFileNameandmakePortableDirNameincorrectly stated that invalid characters were removed, when they are in fact replaced with _. Comments have been corrected to accurately describe the sanitization behavior.Tests
Added test cases covering reserved device name handling in
makePortableName(bare names such as "con", "NUL", "com0") and the newisValidFileNameCharandisValidFileNamefunctions.