abalaji added a comment.
Added a couple inline comments for you to address. Also, please use four
spaces instead of tabs, and keep the spacing consistent (spaces around `+`,
space between `if ()` and the `{). I have yet to run and test this, but if you
check the comment on the left side of the diff, I wonder if just removing that
`.toLower()` is enough.
INLINE COMMENTS
> batchrenamejob.cpp:62
> QSet<QString> extensions;
> - QMimeDatabase db;
> + const QString dot = QString::fromStdString(".");
> foreach (const QUrl &url, m_srcList) {
You can just `QString dot = "."`, but you should use a single character instead
of a string, since it's just a single character
> batchrenamejob.cpp:64
> foreach (const QUrl &url, m_srcList) {
> - const QString extension =
> db.suffixForFileName(url.toDisplayString().toLower());
> if (extensions.contains(extension)) {
I wonder if just getting rid of `.toLower()` fixes this bug
> batchrenamejob.cpp:68
> + //If the oldFileName contains a '.' then its extension is all to
> the right of the last dot
> + if (urlStr.contains(dot)){
> + extension = urlStr.mid(urlStr.lastIndexOf(dot)+1);
Rather than using `contains` here, you can just use `lastIndexOf`, check if
it's not `-1`, and go from there. Just simply `urlStr.lastIndexOf('.')`.
> batchrenamejob.cpp:179
> +
> + const QString dot = QString::fromStdString(".");
> + const QString oldFileName = oldUrl.fileName();
Same as above
> batchrenamejob.cpp:184
> + //If the oldFileName contains a '.' then its extension is all to the
> right of the last dot
> + if (oldFileName.contains(dot)){
> + extension = oldFileName.mid(oldFileName.lastIndexOf(dot)+1);
Same as above
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D18915
To: cfoster, #dolphin, #frameworks, abalaji
Cc: chinmoyr, kde-frameworks-devel, michaelh, ngraham, bruns
