Changeset View
Standalone View
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) { | ||
abalaji: You can just `QString dot = "."`, but you should use a single character instead of a string… | |||||
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. | |||||
elvisangelaccio: Please use `QStringLiteral()` instead. | |||||
64 | const QString extension = db.suffixForFileName(url.toDisplayString().toLower()); | 63 | const QString extension = GetFileExtension(url.fileName()); | ||
abalaji: 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. chinmoyr: I agree with @abalaji here. Modifying the code around this line should fix the bug and also… | |||||
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: 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 | } | ||
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… | |||||
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 GetFileExtension(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 Lines | 136 | { | |||
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 | ||||
bruns: Space after `//` | |||||
161 | // This function is a helper which avoids the conversion to lower case | ||||
bruns: helper, not wrapper | |||||
bruns: `co_n_version` | |||||
162 | QString BatchRenameJobPrivate::GetFileExtension(QString filename) | ||||
163 | { | ||||
164 | QMimeDatabase db; | ||||
165 | const size_t extensionLen = db.suffixForFileName(filename).length(); | ||||
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. | |||||
Just making sure I understand you correctly. cfoster: Just making sure I understand you correctly.
Are you saying I should change the function to… | |||||
QString extension = GetFileExtension(url.fileName()); bruns: `QString extension = GetFileExtension(url.fileName());`
...
`static QString… | |||||
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`… | |||||
cfeck: Actually, const reference:
`const QString &filename` | |||||
166 | | ||||
167 | QString extension; | ||||
168 | //extensionLen will be 0 if the extension is not in the QMimeDatabase i.e "foo.xyz" | ||||
169 | if (extensionLen) { | ||||
170 | extension = filename.right(extensionLen); | ||||
bruns: You can just return here. | |||||
171 | } else { | ||||
172 | //Fallback to everything right of the last '.' | ||||
173 | const int dotIndex = filename.lastIndexOf(QStringLiteral(".")); | ||||
174 | if (dotIndex != -1) { | ||||
175 | extension = filename.mid(dotIndex+1); | ||||
bruns: just return | |||||
176 | } | ||||
177 | } | ||||
bruns: This should be `return QString();`, wont get any cheaper than that. | |||||
178 | return extension; | ||||
179 | } | ||||
180 | | ||||
160 | void BatchRenameJobPrivate::slotStart() | 181 | void BatchRenameJobPrivate::slotStart() | ||
161 | { | 182 | { | ||
162 | Q_Q(BatchRenameJob); | 183 | Q_Q(BatchRenameJob); | ||
163 | 184 | | |||
164 | if (m_listIterator == m_srcList.constBegin()) { // emit total | 185 | if (m_listIterator == m_srcList.constBegin()) { // emit total | ||
165 | q->setTotalAmount(KJob::Files, m_srcList.count()); | 186 | q->setTotalAmount(KJob::Files, m_srcList.count()); | ||
166 | } | 187 | } | ||
167 | 188 | | |||
168 | if (m_listIterator != m_srcList.constEnd()) { | 189 | if (m_listIterator != m_srcList.constEnd()) { | ||
169 | QString newName = indexedName(m_newName, m_index, m_placeHolder); | 190 | QString newName = indexedName(m_newName, m_index, m_placeHolder); | ||
170 | const QUrl oldUrl = *m_listIterator; | 191 | const QUrl oldUrl = *m_listIterator; | ||
171 | QMimeDatabase db; | 192 | const QString extension = GetFileExtension(oldUrl.fileName()); | ||
172 | const QString extension = db.suffixForFileName(oldUrl.path().toLower()); | | |||
173 | if (!extension.isEmpty()) { | 193 | if (!extension.isEmpty()) { | ||
abalaji: Same as above | |||||
174 | newName += QLatin1Char('.') + extension; | 194 | newName += QLatin1Char('.') + extension; | ||
175 | } | 195 | } | ||
176 | 196 | | |||
ngraham: Is it?
`file.tar.gz` | |||||
Ah yes... very good point... cfoster: Ah yes... very good point...
I think have to rethink how to solve this. | |||||
177 | m_newUrl = oldUrl.adjusted(QUrl::RemoveFilename); | 197 | m_newUrl = oldUrl.adjusted(QUrl::RemoveFilename); | ||
178 | m_newUrl.setPath(m_newUrl.path() + KIO::encodeFileName(newName)); | 198 | m_newUrl.setPath(m_newUrl.path() + KIO::encodeFileName(newName)); | ||
abalaji: Same as above | |||||
179 | 199 | | |||
180 | KIO::Job * job = KIO::moveAs(oldUrl, m_newUrl, KIO::HideProgressInfo); | 200 | KIO::Job * job = KIO::moveAs(oldUrl, m_newUrl, KIO::HideProgressInfo); | ||
181 | job->setParentJob(q); | 201 | job->setParentJob(q); | ||
182 | q->addSubjob(job); | 202 | q->addSubjob(job); | ||
183 | q->setProcessedAmount(KJob::Files, q->processedAmount(KJob::Files) + 1); | 203 | q->setProcessedAmount(KJob::Files, q->processedAmount(KJob::Files) + 1); | ||
184 | } else { | 204 | } else { | ||
185 | q->emitResult(); | 205 | q->emitResult(); | ||
186 | } | 206 | } | ||
Show All 27 Lines |
You can just QString dot = ".", but you should use a single character instead of a string, since it's just a single character