Files renamed with Dolphins batch rename would have their file
extensions always converted to lower case.
New method of getting the file extension may not work with a right
to left language such as Arabic
BUG: 402388
abalaji | |
dfaure |
Dolphin | |
Frameworks |
Files renamed with Dolphins batch rename would have their file
extensions always converted to lower case.
New method of getting the file extension may not work with a right
to left language such as Arabic
BUG: 402388
No Linters Available |
No Unit Test Coverage |
Buildable 9812 | |
Build 9830: arc lint + arc unit |
@cfoster Thanks for the patch :)
There's one more mass-rename related bug here:
It is possible to add code for preserving the unknown extension in this patch itself. If not you can always create a new diff :)
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.
src/core/batchrenamejob.cpp | ||
---|---|---|
61–62 | You can just QString dot = ".", but you should use a single character instead of a string, since it's just a single character | |
64 | I wonder if just getting rid of .toLower() fixes this bug | |
67 | Rather than using contains here, you can just use lastIndexOf, check if it's not -1, and go from there. Just simply urlStr.lastIndexOf('.'). | |
192 | Same as above | |
197 | Same as above |
src/core/batchrenamejob.cpp | ||
---|---|---|
61–62 | Without the fromStdString() I get the following compilation error. src/core/batchrenamejob.cpp:64:15: error: 'QString::QString(const char*)' is private within this context QString dot= "."; I must admit I don't fully understand the compilation error however adding fromStdString seemed the most reasonable way to fix it. | |
64 | I tested just removing the .toLower() however this didn't fix it. It seems that QMimeDatabase::suffixForFileName() converts it to lower case unless I overlooked something. See the code snippet below. I tested this with the following lines in a standalone program: | |
67 | Fair point will do. |
src/core/batchrenamejob.cpp | ||
---|---|---|
61–62 | Please use QStringLiteral() instead. |
src/core/batchrenamejob.cpp | ||
---|---|---|
195 | Is it? file.tar.gz |
src/core/batchrenamejob.cpp | ||
---|---|---|
195 | Ah yes... very good point... |
https://doc.qt.io/qt-5/qmimedatabase.html#suffixForFileName
Returns the suffix for the file fileName, as known by the MIME database.
This allows to pre-select "tar.bz2" for foo.tar.bz2, but still only "txt" for my.file.with.dots.txt.
This was the original method of getting the file extension however it was converting the file extensions to lower case.
This can be demonstrated with the following code:
QMimeDatabase db;
QUrl url = (QString) "foo.TXT";
const QString extension = db.suffixForFileName(url.toDisplayString());
qDebug(qUtf8Printable(extension)); //outputs "txt"
Just compare the extension after .toLower(), and if both match, keep the original one.
The following (applied to the original code) should do the trick:
const QString lcExtension = db.suffixForFileName(url.toDisplayString()); // use extension with lower/uppercase from the original file name. const QString extension = url.toDisplayString().right(lcExtension.size());
src/core/batchrenamejob.cpp | ||
---|---|---|
165 | Just making sure I understand you correctly. |
src/core/batchrenamejob.cpp | ||
---|---|---|
165 | QString extension = GetFileExtension(url.fileName()); |
src/core/batchrenamejob.cpp | ||
---|---|---|
165 | Function/method names are usually lowercase. Also, we don't add get for getters, only set for setters. ⇒ fileExtension() ? Additionally, we pass QString via reference ⇒ QString &filename ? |
src/core/batchrenamejob.cpp | ||
---|---|---|
165 | Actually, const reference: const QString &filename |
Now, a unit test would be nice ...
src/core/batchrenamejob.cpp | ||
---|---|---|
177 | This should be return QString();, wont get any cheaper than that. |
Good catch, but then this is a bug in QMimeDatabase::suffixForFileName.
I just submitted a fix at https://codereview.qt-project.org/c/qt/qtbase/+/274548 -- aiming at the 5.14 branch.
So this commit is still valid of course, but it would be good to use #if QT_VERSION < QT_VERSION_CHECK(5, 14, 0) around the workaround, so that it goes away once we use a Qt version with the fix.
AFAICS the workaround is only the use of "extensionLen", though (that's what should be ifdef'ed). This commit doesn't only fix casing, it also fixes the case of unknown extensions. That's valid and can be kept, no matter which Qt version we're using.
I don't understand the comment about right-to-left languages in the commit log; this isn't using any locale-dependent APIs, is it?
Let's see which branch this ends up in, don't use 5, 14, 0 just yet, it might end up in 5.13.2.
The fix landed in dev.
I now uploaded a backport to 5.15 for review.
https://codereview.qt-project.org/c/qt/qtbase/+/302532
The fix will be in 5.15.1.
Can you add Qt version checks if you intend to keep the KIO workaround?
Or we just say it's fixed in Qt and we discard this. Your choice. But the unittest would make sense to keep (with Qt version checks, if there's no workaround).