Fix: trim whitespace in tileset name when renaming#4495
Fix: trim whitespace in tileset name when renaming#4495Praagya26 wants to merge 1 commit intomapeditor:masterfrom
Conversation
|
😅 you can actually just push extra commits to your existing branch for #4486. It’ll update that PR automatically so we can keep all these whitespace fixes in one place for Bjorn to review. It saves everyone some clicks if we group them! (let's see what bjorn's take on naming validation idea, though implementing that won't be a cup of tea ) |
bjorn
left a comment
bjorn
left a comment
There was a problem hiding this comment.
Whitespace-only names are rejected
This change does nothing to reject whitespace-only names. For that, the trim would need to be applied before constructing a RenameTileset command, for example before the isEmpty check in NewTilesetDialog::updateOkButton.
But as-is, you can no longer type a tileset name with a space in the Name field in the Properties view, for example, because the space is immediately trimmed. The automatic trimming of the name might also be unexpected when setting the name from scripts.
So I'd suggest to implement this only as a UI change, so that it can properly respond:
- In
NewTilesetDialog::updateOkButton, check whether the string is empty after trimming. Probably also inNewTilesetDialog::nameEdited. - Trim the name in
NewTilesetDialog::tryAccept. - If we want to disallow trailing and leading whitespace in the Properties view, we should probably use a
QValidator. Currently theStringPropertydoes not support this, but it could support setting a customTrimValidatorthat accepts trailing whitespace asIntermediateand trims it infixup.
| void TilesetDocument::setTilesetName(const QString &name) | ||
| { | ||
| mTileset->setName(name); | ||
| mTileset->setName(name.trimmed()); |
There was a problem hiding this comment.
Since setTilesetName is only called from RenameTileset, this change seems superfluous. The only thing it changes, is that the previous name will also be trimmed on undoing a tileset rename. But I don't think that's what we want.
3 participants
Problem:
Fix:
Applied QString::trimmed() to sanitize input:
In tilesetchanges.cpp using newName.trimmed()
In tilesetdocument.cpp using name.trimmed()
This ensures:
Whitespace-only names are rejected.
Leading and trailing spaces are removed before further processing.
Result:
Prevents invalid or empty tileset names.
Improves consistency and reliability of naming behavior.
Future Work:
As suggested by @kunal649, consider introducing a CENTRALIZED VALIDATION UTILITY.
This would help enforce consistent naming rules across the codebase and reduce duplication.