feat: replace spaces by another delimiter in renamer #1827
Conversation
|
Hi! First of all: THANK YOU for opening a PR! That is very much welcome and I'm glad other people are interested in developing MediaElch! Due to the size of this PR, I will likely not get to review this this week. See #1710 for reasons (time constraints). The renamer is old and not well written. The fact that you had to adapt so many places is a sign for that. :) Do you want me to change minor stuff like renaming Regards, |
src/renamer/Renamer.cpp
Outdated
| @@ -71,6 +71,15 @@ QString Renamer::replaceCondition(QString& text, const QString& condition, bool | |||
| return text; | |||
| } | |||
|
|
|||
| bool Renamer::replaceDelimiter(QString& text, QString oldDelimiter, QString newDelimiter, bool execCondition) | |||
There was a problem hiding this comment.
The third argument seems strange. I think it would be better to remove it and move the boolean into a condition surrounding the call to replaceDelimiter:
if (execCondition) {
Renamer::replaceDelimiter(...)
}
Reason being: It is not clear to the caller what the third argument does.
Also: `sanitizeFolderName` already calls `sanitizeFileName` in your PR. Why call `sanitizeFileName` after `replaceDelimiter` in `*Renamer.cpp`? :)There was a problem hiding this comment.
Hello, I made some modifications based on the feedback you gave me. If there are other things that need to be changed, please let me know, and I will try to do it as soon as possible ! You can add code comments if that works best for you. ^^.
… after + Remove bool condition
src/globals/Helper.cpp
Outdated
| @@ -228,11 +228,12 @@ void sanitizeFileName(QString& fileName) | |||
| fileName.replace(QRegularExpression(R"(\s\s+)"), " "); | |||
|
|
|||
| fileName = fileName.trimmed(); | |||
| Renamer::replaceDelimiter(fileName, " ", defaultDelimiter); | |||
There was a problem hiding this comment.
The function is just a text.replace() call. The helper file shouldn't depend on the Renamer one. It would be best to replace this call by the function's contents. :)
src/globals/Helper.cpp
Outdated
| } | ||
|
|
||
| void sanitizeFolderName(QString& fileName) | ||
| void sanitizeFolderName(QString& fileName, QString defaultDelimiter) |
There was a problem hiding this comment.
I'd prefer const QString&, as we don't need the string to be mutable. :)
| <item row="1" column="1"> | ||
| <widget class="PlaceholderLineEdit" name="fileNaming"> | ||
| <property name="currentText"> | ||
| <string notr="true"><title>.<extension></string> |
src/settings/Settings.cpp
Outdated
| @@ -847,6 +876,13 @@ void Settings::renamings(RenameType renameType, bool& files, bool& folders, bool | |||
| seasonDirectories = | |||
| settings()->value(QString("RenamePattern/%1/UseSeasonDirectories").arg(renameTypeStr), true).toBool(); | |||
| } | |||
| void Settings::renamings(RenameType renameType, bool& files, bool& folders, bool& seasonDirectories, bool& replaceDelimiter) | |||
There was a problem hiding this comment.
There is no need to add another method. Adapting the existing one is fine. :)
The code isn't great to begin with, so adding another argument is ok here.
| { | ||
| Settings::setRenamings(renameType, files, folders, seasonDirectories); | ||
| const QString renameTypeStr = renamerTypeToString(renameType); | ||
| settings()->setValue(QString("RenamePattern/%1/ReplaceDelimiter").arg(renameTypeStr), replaceDelimiter); |
There was a problem hiding this comment.
I'd prefer Delimiter, because that's what it is intended to be: a delimiter between words. That we only replace spaces is a restriction or implementation detail.
There was a problem hiding this comment.
Ah, this comment was meant for the string value, not the boolean value. :)
|
Thank you for your feedback, the correcting and merging the pr ! I will take note of those for future contributions. |
add feature from request #1082