Fix batchrename changing extension to lower case
Needs RevisionPublic

Authored by cfoster on Feb 10 2019, 11:19 PM.

Details

Reviewers
abalaji
dfaure
Group Reviewers
Dolphin
Frameworks
Summary

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

Test Plan
  1. Open Dolphin
  2. Create files "foo.TXT" and "bar.TXT"
  3. Batch rename (F2) to name "x"
  4. Check files are now named "x1.TXT" and "x2.TXT"

Diff Detail

Repository
R241 KIO
Branch
arcpatch-D18915
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 9812
Build 9830: arc lint + arc unit
cfoster created this revision.Feb 10 2019, 11:19 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 10 2019, 11:19 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
cfoster requested review of this revision.Feb 10 2019, 11:19 PM
ngraham edited the summary of this revision. (Show Details)Feb 11 2019, 1:06 AM
ngraham added reviewers: Dolphin, Frameworks, abalaji.

@cfoster Thanks for the patch :)

There's one more mass-rename related bug here:

  1. Create two files; one with a known extension (.cpp) and the other with a random extension (.xyz, anything not in mime db)
  2. Rename both.
  3. The extension (.xyz) is removed from second file.

It is possible to add code for preserving the unknown extension in this patch itself. If not you can always create a new diff :)

abalaji added a comment.EditedFeb 11 2019, 3:56 AM

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

chinmoyr added inline comments.Feb 11 2019, 4:19 AM
src/core/batchrenamejob.cpp
64

I agree with @abalaji here. Modifying the code around this line should fix the bug and also address my previous comment.

cfoster added inline comments.Feb 11 2019, 8:14 AM
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:
QMimeDatabase db;
QUrl url = (QString) "foo.TXT";
const QString extension = db.suffixForFileName(url.toDisplayString());
qDebug(qUtf8Printable(extension)); //Outputs "txt"

67

Fair point will do.

elvisangelaccio added inline comments.
src/core/batchrenamejob.cpp
61–62

Please use QStringLiteral() instead.

cfoster updated this revision to Diff 52180.Feb 20 2019, 9:44 PM
  • Changes after review feedback
ngraham added inline comments.
src/core/batchrenamejob.cpp
195

Is it?

file.tar.gz

cfoster added inline comments.Feb 20 2019, 9:49 PM
src/core/batchrenamejob.cpp
195

Ah yes... very good point...
I think have to rethink how to solve this.

bruns added a subscriber: bruns.Feb 20 2019, 10:46 PM

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.

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"

bruns added a comment.EditedFeb 20 2019, 10:55 PM

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());

And factor out the function, it is used in 2 places, and you can unit test it then.

cfoster updated this revision to Diff 52691.Feb 26 2019, 11:47 PM
  • Add function for getting extension All unit tests pass Manually tested invalid extensions. Files with no extensions and files with upper and lower case extensions
bruns added inline comments.Feb 27 2019, 1:16 AM
src/core/batchrenamejob.cpp
160

Space after //

161

helper, not wrapper

161

co_n_version

165

I think you can use QUrl::filename(), and you should do it from the calling code.

cfoster added inline comments.Feb 27 2019, 10:42 PM
src/core/batchrenamejob.cpp
165

Just making sure I understand you correctly.
Are you saying I should change the function to take the filename as a QString and extract the filename from the QUrl outside of the function or change url.toDisplayString() to url.fileName() within the function?

bruns added inline comments.Feb 28 2019, 12:39 AM
src/core/batchrenamejob.cpp
165

QString extension = GetFileExtension(url.fileName());
...
static QString BatchRenameJobPrivate::GetFileExtension(QString filename)

cfoster marked 6 inline comments as done.Mar 3 2019, 4:36 PM
cfoster updated this revision to Diff 53068.Mar 3 2019, 4:36 PM
  • Change GetFileExtension to take QString
cfeck added a subscriber: cfeck.Mar 8 2019, 8:51 PM
cfeck added inline comments.
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 ?

cfeck added inline comments.Mar 8 2019, 8:53 PM
src/core/batchrenamejob.cpp
165

Actually, const reference:

const QString &filename

bruns added inline comments.Mar 8 2019, 9:13 PM
src/core/batchrenamejob.cpp
170

You can just return here.

175

just return

cfoster updated this revision to Diff 53914.Mar 14 2019, 7:37 PM
  • Pass QString by reference

Now, a unit test would be nice ...

src/core/batchrenamejob.cpp
177

This should be return QString();, wont get any cheaper than that.

cfoster updated this revision to Diff 54269.Mar 18 2019, 9:24 PM
  • Add unit tests.

What needs to happen now for this to be accepted?

dfaure requested changes to this revision.Sep 19 2019, 1:34 PM

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?

This revision now requires changes to proceed.Sep 19 2019, 1:34 PM

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

dfaure added a comment.Jun 2 2020, 6:53 PM

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).