D18915: Fix batchrename changing extension to lower case

dfaure added a comment. 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). REPOS

D18915: Fix batchrename changing extension to lower case

dfaure added a comment. The fix landed in dev. I now uploaded a backport to 5.15 for review. https://codereview.qt-project.org/c/qt/qtbase/+/302532 REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D18915 To: cfoster, #dolphin, #frameworks, abalaji, dfaure Cc: cfeck,

D18915: Fix batchrename changing extension to lower case

dfaure added a comment. 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. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D18915 To: cfoster, #dolphin, #frameworks, abalaji, dfaure Cc: cfeck, bruns, ngraham, elvisangelaccio,

D18915: Fix batchrename changing extension to lower case

dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. 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

D18915: Fix batchrename changing extension to lower case

ahmadsamir added a reviewer: dfaure. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D18915 To: cfoster, #dolphin, #frameworks, abalaji, dfaure Cc: cfeck, bruns, ngraham, elvisangelaccio, chinmoyr, kde-frameworks-devel, LeGast00n, GB_2, michaelh

D18915: Fix batchrename changing extension to lower case

cfoster added a comment. What needs to happen now for this to be accepted? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D18915 To: cfoster, #dolphin, #frameworks, abalaji Cc: cfeck, bruns, ngraham, elvisangelaccio, chinmoyr, kde-frameworks-devel, michaelh

D18915: Fix batchrename changing extension to lower case

bruns added a comment. LGTM REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D18915 To: cfoster, #dolphin, #frameworks, abalaji Cc: cfeck, bruns, ngraham, elvisangelaccio, chinmoyr, kde-frameworks-devel, michaelh

D18915: Fix batchrename changing extension to lower case

cfoster updated this revision to Diff 54269. cfoster added a comment. - Add unit tests. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D18915?vs=53914&id=54269 BRANCH arcpatch-D18915 REVISION DETAIL https://phabricator.kde.org/D18915 AFFECTED FILES auto

D18915: Fix batchrename changing extension to lower case

bruns added a comment. Now, a unit test would be nice ... INLINE COMMENTS > batchrenamejob.cpp:177 > +} > +return QStringLiteral(); > +} This should be `return QString();`, wont get any cheaper than that. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D18915 T

D18915: Fix batchrename changing extension to lower case

cfoster updated this revision to Diff 53914. cfoster added a comment. - Pass QString by reference REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D18915?vs=53068&id=53914 BRANCH arcpatch-D18915 REVISION DETAIL https://phabricator.kde.org/D18915 AFFECTED FI

D18915: Fix batchrename changing extension to lower case

bruns added inline comments. INLINE COMMENTS > batchrenamejob.cpp:170 > +if (extensionLen) { > +extension = filename.right(extensionLen); > +} else { You can just return here. > batchrenamejob.cpp:175 > +if (dotIndex != -1) { > +extension = filename.mid(dotIn

D18915: Fix batchrename changing extension to lower case

cfeck added inline comments. INLINE COMMENTS > cfeck wrote in 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 &filenam

D18915: Fix batchrename changing extension to lower case

cfeck added inline comments. INLINE COMMENTS > bruns wrote in batchrenamejob.cpp:165 > `QString extension = GetFileExtension(url.fileName());` > ... > `static QString BatchRenameJobPrivate::GetFileExtension(QString filename)` Function/method names are usually lowercase. Also, we don't add `get`

D18915: Fix batchrename changing extension to lower case

cfoster updated this revision to Diff 53068. cfoster added a comment. - Change GetFileExtension to take QString REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D18915?vs=52691&id=53068 BRANCH arcpatch-D18915 REVISION DETAIL https://phabricator.kde.org/D1891

D18915: Fix batchrename changing extension to lower case

cfoster marked 6 inline comments as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D18915 To: cfoster, #dolphin, #frameworks, abalaji Cc: bruns, ngraham, elvisangelaccio, chinmoyr, kde-frameworks-devel, michaelh

D18915: Fix batchrename changing extension to lower case

bruns added inline comments. INLINE COMMENTS > cfoster wrote in 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.

D18915: Fix batchrename changing extension to lower case

cfoster added inline comments. INLINE COMMENTS > bruns wrote in batchrenamejob.cpp:165 > I think you can use `QUrl::filename()`, and you should do it from the calling > code. Just making sure I understand you correctly. Are you saying I should change the function to take the filename as a QStri

D18915: Fix batchrename changing extension to lower case

bruns added inline comments. INLINE COMMENTS > batchrenamejob.cpp:160 > > +//The function QMimeDatabase.suffixForFileName always converts the extension > to lower case > +//This function is a wrapper which avoids the coversion to lower case Space after `//` > batchrenamejob.cpp:161 > +//The

D18915: Fix batchrename changing extension to lower case

cfoster updated this revision to Diff 52691. cfoster added a comment. - 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 REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE

D18915: Fix batchrename changing extension to lower case

bruns added a comment. And factor out the function, it is used in 2 places, and you can unit test it then. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D18915 To: cfoster, #dolphin, #frameworks, abalaji Cc: bruns, ngraham, elvisangelaccio, chinmoyr, kde-frameworks-dev

D18915: Fix batchrename changing extension to lower case

bruns added a comment. In D18915#416402 , @cfoster wrote: > In D18915#416400 , @bruns wrote: > > > https://doc.qt.io/qt-5/qmimedatabase.html#suffixForFileName > > > > > Returns the suffix for the

D18915: Fix batchrename changing extension to lower case

cfoster added a comment. In D18915#416400 , @bruns wrote: > 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" f

D18915: Fix batchrename changing extension to lower case

bruns added a comment. 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. REPOSITORY R241 KIO R

D18915: Fix batchrename changing extension to lower case

cfoster added inline comments. INLINE COMMENTS > ngraham wrote in batchrenamejob.cpp:181 > Is it? > > `file.tar.gz` Ah yes... very good point... I think have to rethink how to solve this. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D18915 To: cfoster, #dolphin, #frame

D18915: Fix batchrename changing extension to lower case

ngraham added inline comments. INLINE COMMENTS > batchrenamejob.cpp:181 > + > + //If the oldFileName contains a '.' then its extension is all to the > right of the last dot > +int dotIndex = oldFileName.lastIndexOf(QStringLiteral(".")); Is it? `file.tar.gz` REPOSITORY R241 KIO

D18915: Fix batchrename changing extension to lower case

cfoster updated this revision to Diff 52180. cfoster added a comment. - Changes after review feedback REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D18915?vs=51372&id=52180 BRANCH arcpatch-D18915_2 REVISION DETAIL https://phabricator.kde.org/D18915 AFFEC

D18915: Fix batchrename changing extension to lower case

elvisangelaccio added inline comments. INLINE COMMENTS > cfoster wrote in 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=

D18915: Fix batchrename changing extension to lower case

cfoster added inline comments. INLINE COMMENTS > abalaji wrote in 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 Without the fromStdString() I get the following compilation error. src/cor

D18915: Fix batchrename changing extension to lower case

chinmoyr added inline comments. INLINE COMMENTS > abalaji wrote in batchrenamejob.cpp:64 > I wonder if just getting rid of `.toLower()` fixes this bug I agree with @abalaji here. Modifying the code around this line should fix the bug and also address my previous comment. REPOSITORY R241 KIO

D18915: Fix batchrename changing extension to lower case

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 t

D18915: Fix batchrename changing extension to lower case

chinmoyr added a comment. @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 remove

D18915: Fix batchrename changing extension to lower case

ngraham edited the summary of this revision. ngraham added reviewers: Dolphin, Frameworks, abalaji. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D18915 To: cfoster, #dolphin, #frameworks, abalaji Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D18915: Fix batchrename changing extension to lower case

cfoster created this revision. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. cfoster requested review of this revision. REVISION SUMMARY Files renamed with Dolphins batch rename would have their file extensions always converted to lower case. New met