This job will be used to rename multiple files. It basically reuses the code from Dolphin::RenameDialog.
Details
Details
Diff Detail
Diff Detail
- Repository
- R241 KIO
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
Wouldn't it be easier to have this job in Dolphin? Or it needs to be in kio to access private API?
Comment Actions
I'm OK with this being in KIO. One day we might want the same from the file dialog, or krusader, etc.
| autotests/batchrenamejobtest.cpp | ||
|---|---|---|
| 21 | this is all of QtTest + all of QtCore, please use <QTest> instead | |
| 52 | srcList.reserve(fileList.count()); | |
| src/core/batchrenamejob.cpp | ||
| 49 | Well soon this won't be dolphin's logic anymore, right? Anyone reading this in 2 years won't find such logic in dolphin but only here? Then don't point to Dolphin ;) | |
| 52 | "At least" is two words. | |
| 56 | Why special case "not a sequence", rather than just replacing the first (last?) sequence? | |
| 65 | cheap to create, but still, move outside foreach | |
| 76 | this code iterates over newName's 3 times... I wonder why this isn't just indexOf + a small loop until (current char != placeholder). Simpler, faster, and easier to specify (as mentioned above) | |
| 110 | nice docu, wrong location :-) | |
| 154 | Faster: indexString.prepend(QString(minIndexLength - indexString.length(), QLatin1Char('0')); | |
| 159 | what if that's -1 because not found? can this happen? | |
| 197 | what happens if we get "file already exists"? It seems the error is ignored? | |
| src/core/batchrenamejob.h | ||
| 47 | add a one-line docu | |
| 53 | /// @internal | |
| 66 | Can you add an example here, to make it clearer? Also mention that newName *must* contain the indexPlaceHolder character (right?) (what happens if it doesn't?) Also I find the name "indexPlaceHolder" misleading, it reads like "index of the placeholder". Why not just call it placeHolder? (or replacementCharacter?) | |
| 72 | QChar is basically a quint16, pass it by value, not by const ref. | |
| src/core/batchrenamejob.cpp | ||
|---|---|---|
| 56 | Not done, the comment here still talks about the "invalid because not connected sequence" case. Instead, point 3 could be simplified to "no placeholder", i.e. no longer "invalid" that needs to be explained with "(This means...)". But of course the code also needs to do this: stop at the first sequence of placeholders, use that. | |
| 57 | At least is two words in this sentence too ;) | |
| 76 | not done. You added an if(), but you didn't implement my suggestion. In a hurry? ;) | |
| 87 | coding style: join with previous line | |
| 197 | ? | |
| src/core/batchrenamejob.h | ||
| 47 | not done | |
| 53 | not done | |
| 66 | not done | |
Comment Actions
Hopefully I haven't missed any more of your comments.
| src/core/batchrenamejob.cpp | ||
|---|---|---|
| 56 | Because there might not be a first or last sequence present. In case 3 we don't need a replacement and in case 4 we just append the integer irrespective of placeholder's location. | |
| 56 | Here if there is exactly one sequence of placeholders then it is valid otherwise invalid. | |
| 76 | Here it is searching for exactly one sequence. If more than one sequence are present like $$file$$name$$ then it is considered invalid. | |
| 76 | Not really in a hurry :P I missed two comments and I also hoped arc would submit my comments but it didn't. | |
| 110 | I just copied whatever there was in Dolphin. Where should I move it to? | |
| 159 | This cannot happen. m_useIndex and m_appendIndex are there for that purpose only. | |
| 197 | In dolphin the overwrite dialog pops up. Otherwise the fileRenamed is still emitted and the job continues. Maybe I should make that its emission conditional? | |
| src/core/batchrenamejob.h | ||
| 53 | I hope it is for BatchRenameJob. | |
| 66 | It is not necessary for newName to contain the placeholder. If its not there index will be appended to newName. | |
| src/core/batchrenamejob.cpp | ||
|---|---|---|
| 76 | But why? Why doesn't this lead to 12file$$name$$? It's a weird input in the first place, I don't mind if the output is weird, because the benefit of doing it that way is:
OK I see that you solved the latter point in this commit though, it's at least fast now. Then I don't mind as much. You can leave it as is, I just wanted to finish explaining my point ;) | |
| 77 | The trailing ';' looks like a mistake, even if it's not. {} solves this, or moving ++first to the body of the while (more readable IMHO). BTW if it moves forward it shouldn't be called first, it makes the indexOf below very confusing. Maybe just call it pos. | |
| 110 | The public header, where people will see it ;) I see you did that now, thanks. | |
| 197 | Ah right KIO::moveAs itself pops up the overwrite dialog, but then can it end with KIO::ERR_FILE_ALREADY_EXIST ? At that point the user should overwrite or skip or cancel. Are you really sure this error code can happen in this slot? | |
| src/core/batchrenamejob.h | ||
| 73 | ... and then this should mention the case where no placeholder is present, and 12 gets appended (before the extension, IIUC). | |
| src/core/batchrenamejob.cpp | ||
|---|---|---|
| 197 | If the job isn't interactive (like in unit test) then it ends with ERR_FILE_ALREADY_EXIST error. In case of unit test ignoring the error seems quite harmless. | |
| src/core/batchrenamejob.cpp | ||
|---|---|---|
| 197 | Ah, yes, makes sense when non-interactive. But this still reads like "risky" code to me. Why not remove the special check? For unittests it means any "existing dest" situation will lead to an error that aborts the whole job, but that's good, no? | |