Fix batchrename changing extension to lower case
Needs ReviewPublic

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

Details

Reviewers
abalaji
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
FileExtensionCase (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 8135
Build 8153: arc lint + arc unit
cfoster created this revision.Sun, Feb 10, 11:19 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptSun, Feb 10, 11:19 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
cfoster requested review of this revision.Sun, Feb 10, 11:19 PM
ngraham edited the summary of this revision. (Show Details)Mon, Feb 11, 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.EditedMon, Feb 11, 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
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

68

Rather than using contains here, you can just use lastIndexOf, check if it's not -1, and go from there. Just simply urlStr.lastIndexOf('.').

179

Same as above

184

Same as above

chinmoyr added inline comments.Mon, Feb 11, 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.Mon, Feb 11, 8:14 AM
src/core/batchrenamejob.cpp
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"

68

Fair point will do.

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

Please use QStringLiteral() instead.