Differential D18915 Diff 54269 src/core/batchrenamejob.cpp

Changeset View

Standalone View

View Options

src/core/batchrenamejob.cpp

Show First 20 Lines • Show All 52 Lines • ▼ Show 20 Line(s)47 {
53 // (this means either $newName doesn't contain the placeholder or the placeholders 53​ // (this means either $newName doesn't contain the placeholder or the placeholders
54 // are not in a connected sequence). 54​ // are not in a connected sequence).
55 // In this case nothing is substituted and all files have the same $newName. 55​ // In this case nothing is substituted and all files have the same $newName.
56 // 4. At least two files have same extension and $newName contains an invalid placeholder. 56​ // 4. At least two files have same extension and $newName contains an invalid placeholder.
57 // In this case $index is appended to $newName. 57​ // In this case $index is appended to $newName.
58 58​
59 59​
60 // Check for extensions. 60​ // Check for extensions.
61 QSet<QString> extensions; 61​ QSet<QString> extensions;
62 QMimeDatabase db; ​
63 foreach (const QUrl &url, m_srcList) { 62​ foreach (const QUrl &url, m_srcList) {
abalajiUnsubmitted
Not Done

You can just QString dot = ".", but you should use a single character instead of a string, since it's just a single character

abalaji: You can just `QString dot = "."`, but you should use a single character instead of a string…
cfosterAuthorUnsubmitted
Not Done

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.

cfoster: Without the fromStdString() I get the following compilation error. src/core/batchrenamejob.
elvisangelaccioUnsubmitted
Not Done

Please use QStringLiteral() instead.

elvisangelaccio: Please use `QStringLiteral()` instead.
64 const QString extension = db.suffixForFileName(url.toDisplayString().toLower()); 63​ const QString extension = fileExtension(url.fileName());
abalajiUnsubmitted
Not Done

I wonder if just getting rid of .toLower() fixes this bug

abalaji: I wonder if just getting rid of `.toLower()` fixes this bug
chinmoyrUnsubmitted
Not Done

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

chinmoyr: I agree with @abalaji here. Modifying the code around this line should fix the bug and also…
cfosterAuthorUnsubmitted
Not Done

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"

cfoster: I tested just removing the .toLower() however this didn't fix it. It seems that QMimeDatabase…
65 if (extensions.contains(extension)) { 64​ if (extensions.contains(extension)) {
66 m_allExtensionsDifferent = false; 65​ m_allExtensionsDifferent = false;
67 break; 66​ break;
68 } 67​ }
abalajiUnsubmitted
Not Done

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

abalaji: Rather than using `contains` here, you can just use `lastIndexOf`, check if it's not `-1`, and…
cfosterAuthorUnsubmitted
Not Done

Fair point will do.

cfoster: Fair point will do.
69 68​
70 extensions.insert(extension); 69​ extensions.insert(extension);
71 } 70​ }
72 71​
73 // Check for exactly one placeholder character or exactly one sequence of placeholders. 72​ // Check for exactly one placeholder character or exactly one sequence of placeholders.
74 int pos = newName.indexOf(placeHolder); 73​ int pos = newName.indexOf(placeHolder);
75 if (pos != -1) { 74​ if (pos != -1) {
76 while (pos < newName.size() && newName.at(pos) == placeHolder) { 75​ while (pos < newName.size() && newName.at(pos) == placeHolder) {
Show All 20 Lines
97 bool m_useIndex; 96​ bool m_useIndex;
98 bool m_appendIndex; 97​ bool m_appendIndex;
99 QUrl m_newUrl; // for fileRenamed signal 98​ QUrl m_newUrl; // for fileRenamed signal
100 const JobFlags m_flags; 99​ const JobFlags m_flags;
101 100​
102 Q_DECLARE_PUBLIC(BatchRenameJob) 101​ Q_DECLARE_PUBLIC(BatchRenameJob)
103 102​
104 void slotStart(); 103​ void slotStart();
104​ static QString fileExtension(const QString& filename);
105 105​
106 QString indexedName(const QString& name, int index, QChar placeHolder) const; 106​ QString indexedName(const QString& name, int index, QChar placeHolder) const;
107 107​
108 static inline BatchRenameJob *newJob(const QList<QUrl> &src, const QString &newName, 108​ static inline BatchRenameJob *newJob(const QList<QUrl> &src, const QString &newName,
109 int index, QChar placeHolder, JobFlags flags) 109​ int index, QChar placeHolder, JobFlags flags)
110 { 110​ {
111 BatchRenameJob *job = new BatchRenameJob(*new BatchRenameJobPrivate(src, newName, index, placeHolder, flags)); 111​ BatchRenameJob *job = new BatchRenameJob(*new BatchRenameJobPrivate(src, newName, index, placeHolder, flags));
112 job->setUiDelegate(KIO::createDefaultJobUiDelegate()); 112​ job->setUiDelegate(KIO::createDefaultJobUiDelegate());
Show All 39 Lines136{
152 152​
153 // Replace the index placeholders by the indexString 153​ // Replace the index placeholders by the indexString
154 const int placeHolderStart = newName.indexOf(placeHolder); 154​ const int placeHolderStart = newName.indexOf(placeHolder);
155 newName.replace(placeHolderStart, minIndexLength, indexString); 155​ newName.replace(placeHolderStart, minIndexLength, indexString);
156 156​
157 return newName; 157​ return newName;
158} 158​}
159 159​
160​// The function QMimeDatabase.suffixForFileName always converts the extension to lower case
brunsUnsubmitted
Done

Space after //

bruns: Space after `//`
161​// This function is a helper which avoids the conversion to lower case
brunsUnsubmitted
Done

helper, not wrapper

bruns: helper, not wrapper
brunsUnsubmitted
Done

co_n_version

bruns: `co_n_version`
162​QString BatchRenameJobPrivate::fileExtension(const QString& filename)
163​{
164​ QMimeDatabase db;
165​ const size_t extensionLen = db.suffixForFileName(filename).length();
brunsUnsubmitted
Done

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

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

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?

cfoster: Just making sure I understand you correctly. Are you saying I should change the function to…
brunsUnsubmitted
Done

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

bruns: `QString extension = GetFileExtension(url.fileName());` ... `static QString…
cfeckUnsubmitted
Not Done

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: Function/method names are usually lowercase. Also, we don't add `get` for getters, only `set`…
cfeckUnsubmitted
Not Done

Actually, const reference:

const QString &filename

cfeck: Actually, const reference: `const QString &filename`
166​
167​ //extensionLen will be 0 if the extension is not in the QMimeDatabase i.e "foo.xyz"
168​ if (extensionLen) {
169​ return filename.right(extensionLen);
170​ } else {
brunsUnsubmitted
Not Done

You can just return here.

bruns: You can just return here.
171​ //Fallback to everything right of the last '.'
172​ const int dotIndex = filename.lastIndexOf(QStringLiteral("."));
173​ if (dotIndex != -1) {
174​ return filename.mid(dotIndex+1);
175​ }
brunsUnsubmitted
Not Done

just return

bruns: just return
176​ }
177​ return QString();
brunsUnsubmitted
Not Done

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

bruns: This should be `return QString();`, wont get any cheaper than that.
178​}
179​
160void BatchRenameJobPrivate::slotStart() 180​void BatchRenameJobPrivate::slotStart()
161{ 181​{
162 Q_Q(BatchRenameJob); 182​ Q_Q(BatchRenameJob);
163 183​
164 if (m_listIterator == m_srcList.constBegin()) { // emit total 184​ if (m_listIterator == m_srcList.constBegin()) { // emit total
165 q->setTotalAmount(KJob::Files, m_srcList.count()); 185​ q->setTotalAmount(KJob::Files, m_srcList.count());
166 } 186​ }
167 187​
168 if (m_listIterator != m_srcList.constEnd()) { 188​ if (m_listIterator != m_srcList.constEnd()) {
169 QString newName = indexedName(m_newName, m_index, m_placeHolder); 189​ QString newName = indexedName(m_newName, m_index, m_placeHolder);
170 const QUrl oldUrl = *m_listIterator; 190​ const QUrl oldUrl = *m_listIterator;
171 QMimeDatabase db; 191​ const QString extension = fileExtension(oldUrl.fileName());
172 const QString extension = db.suffixForFileName(oldUrl.path().toLower()); ​
173 if (!extension.isEmpty()) { 192​ if (!extension.isEmpty()) {
abalajiUnsubmitted
Not Done

Same as above

abalaji: Same as above
174 newName += QLatin1Char('.') + extension; 193​ newName += QLatin1Char('.') + extension;
175 } 194​ }
176 195​
ngrahamUnsubmitted
Not Done

Is it?

file.tar.gz

ngraham: Is it? `file.tar.gz`
cfosterAuthorUnsubmitted
Not Done

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

cfoster: Ah yes... very good point... I think have to rethink how to solve this.
177 m_newUrl = oldUrl.adjusted(QUrl::RemoveFilename); 196​ m_newUrl = oldUrl.adjusted(QUrl::RemoveFilename);
178 m_newUrl.setPath(m_newUrl.path() + KIO::encodeFileName(newName)); 197​ m_newUrl.setPath(m_newUrl.path() + KIO::encodeFileName(newName));
abalajiUnsubmitted
Not Done

Same as above

abalaji: Same as above
179 198​
180 KIO::Job * job = KIO::moveAs(oldUrl, m_newUrl, KIO::HideProgressInfo); 199​ KIO::Job * job = KIO::moveAs(oldUrl, m_newUrl, KIO::HideProgressInfo);
181 job->setParentJob(q); 200​ job->setParentJob(q);
182 q->addSubjob(job); 201​ q->addSubjob(job);
183 q->setProcessedAmount(KJob::Files, q->processedAmount(KJob::Files) + 1); 202​ q->setProcessedAmount(KJob::Files, q->processedAmount(KJob::Files) + 1);
184 } else { 203​ } else {
185 q->emitResult(); 204​ q->emitResult();
186 } 205​ }
Show All 27 Lines