| elvisangelaccio | |
| rthomsen |
| Ark |
This patch is for solving:
Bug 403146 - Extracting suggests replacement file with info of destination file
I have tested with ark 19.04.0. It allows to have size and date of the source file from the archive, not the destination information.
The modifications are :
BUG: 403146
FIXED-IN: 20.12
Create an archive.
Modify a file which copy is in the archive
Open it with ark, extract the archive at the same location. A dialog opens to replace the file. At left, the information (date, size) should be the information before modification, and at right, from the modified file.
Extract or not.
| Lint Skipped |
| Unit Tests Skipped |
Nice first patch @papoteur , seems good to me.
Great on you pushing through your patch to phabricator.
Please fix the few formatting issues.
Then we leave the final say to Ark maintainer @elvisangelaccio
| kerfuffle/queries.cpp | ||
|---|---|---|
| 87 | indentation seems wrong here. | |
| 95 | A space too much | |
| kerfuffle/queries.h | ||
| 72 | Please remove this unneeded end space | |
| 104 | Same | |
Btw, You can mark the comment you treated as "Done" in phabricator, it it eases the review work.
This is what I have done, by ticking the checkbox. Is there something else to do? Do you need to update the page?
I'm not comfortable with the interface and the process. I wonder if the patch is integrated in a test build.
What also confuses me is that bugzilla and phabricator each require their own identity.
Thanks for the patch. This is a good start, but it needs some work.
The problem is we shouldn't use libarchive functions in queries.cpp. It's the wrong layer. We should add new API instead, so that we can call this API from all the plugins (not only libarchive).
So we should add something like setCreationTime() / setModificationTime() to the Query classes and call these new methods from the libarchive plugin. Bonus point if you fix all the plugins of course ;)
Hello,
I tried further. I understood how to create a call to add information needed about source file. However, my calls to this function seem to never occur. With a debugger, I see that calls come from jobs, and I don't understand this mechanism.
I am not familiar with this code base, I would need to see some of you code to see what you mean.
This new patch provides data from source by calling setSourceData.
However, from my tests, it is never called.
This looks nice.
However, from my tests, it is never called.
You might need to edit the other plugins. You could be using another plugin than cliinterface and libarchive without knowing.
The other other code path concerned :
plugins/libzipplugin/libzipplugin.cpp 650: Kerfuffle::OverwriteQuery query(renamedEntry); plugins/libsinglefileplugin/singlefileplugin.cpp 121: Kerfuffle::OverwriteQuery query(newFileName);
Other than that, I can recommend you some good old debugging, to see where the calls stops.
| kerfuffle/queries.cpp | ||
|---|---|---|
| 39 | Can remove | |
| 104 | We use same line { for if and else (with preceeding space). | |
| 105 | sourceUrl can stay out of if/else | |
| 120 | We prefer } else { | |
I think it makes sense to always require specifying ctime, mtime and size, so please add the three values as arguments to the constructor of OverwriteQuery so we can get rid of setSourceData() and m_from_archive.
| kerfuffle/queries.cpp | ||
|---|---|---|
| 44 | The data you set are not used when sourceUrl points to an existing local file (see link). | |
This version modifies the function signature of OverwriteQuery to pass directly data from source file.
Call to the function are adapted in the whole repo. I have tested it with extraction of one single file and a complete archive. Thus I have tested only the call from libarchiveplugin, line 306. I don't know how to test other calls.
Almost ready :)
| kerfuffle/cliinterface.cpp | ||
|---|---|---|
| 598–600 | dirIt.filePath() points to the source file so create a QFileInfo and use that to get the properties. Also please use QFileInfo::birthTime() and QFileInfo::lastModified() here to get ctime and mtime, respectively. | |
| 979 | In this case we can't easily get the properties of the source file, so I guess we should do the same as previously and just send the destination file properties. You should use QDir::current().path() + QLatin1Char( '/' ) + m_storedFileName then. Also please use QFileInfo::birthTime() and QFileInfo::lastModified(). | |
| kerfuffle/queries.h | ||
| 85 | We mostly use camelcase-style in Ark so please call these arguments: ctimeSrc, mtimeSrc and sizeSrc (and the member variables the same just prefixed with m_. | |
| plugins/libsinglefileplugin/singlefileplugin.cpp | ||
| 123–124 | Please use QFileInfo::birthTime() and QFileInfo::lastModified(). | |
| plugins/libzipplugin/libzipplugin.cpp | ||
| 651 | Zip format doesn't store ctime, so guess it makes sense to pass mtime for both. | |
Some nitpicks. I tested the patch and it works as intended :)
| kerfuffle/cliinterface.cpp | ||
|---|---|---|
| 474–477 | Please fix indentation. | |
| 980–985 | No longer needed. | |
| 982 | First argument can now be simplified to: destFile.filePath() | |
| kerfuffle/queries.h | ||
| 85 | You didn't change the name of the arguments and member variables as instructed in my previous comment. Please fix. | |
| plugins/libarchive/libarchiveplugin.cpp | ||
| 307–310 | Fix indentation please. | |
| plugins/libzipplugin/libzipplugin.cpp | ||
| 651–654 | Fix indentation please. | |
| plugins/libarchive/libarchiveplugin.cpp | ||
|---|---|---|
| 307–310 | Sorry, I don't know the rule that can govern the indentation. | |
| plugins/libarchive/libarchiveplugin.cpp | ||
|---|---|---|
| 307–310 | Ok, no worries. Just fix the two other remaining items so we can commit this :) | |
Can you provide full name for commit message or should I use your nickname? And can you provide email-address?
I have tested this patch myself on Arch Linux.
Previews of jpg flles don't work as expected,
the one below "Source" is not generated and below "Destination"
we see the preview of source file.
Ark 20.04.1 from Arch repos shows the thumbnail of source file twice.
Hi, how do you enable preview? I have enabled preview in Dolphin, but this doesn't seem to have effect. I can't get any preview, either for source or destination.
I have done nothing special to enable previews on Arch Linux.
I can confirm missing previews on neon unstable edition despite preview of images in Dolphin is enabled on both systems.
Ark 20.04.1 from Arch repos shows the thumbnail of source file twice.
Hello, I have had a deeper look on how previews are got. Preview are first looked at in a cache directory. Thus, when the source or destination matches an already existing preview, this one is displayed. This explain why your previews are the same for source and destination or are mixed.
If this is a bug, this is not in ark, but in KIO. And it should not prevent to release a fix for this one.
Why not?
Copyright protects the rights of the author. On one side, patrimonial rights allow to give him money, but with open source licence, the author says that he waives to them. On another side, moral right allows him to be cited as author. If he wants to be cited with a nickname, it's also his right.
Well he grants them, but he keeps all his rights, like the ability to change the license to his work that is not transferable through a license but only through a FLA or such.
On another side, moral right allows him to be cited as author. If he wants to be cited with a nickname, it's also his right.
Rights are only applicable to people i.e with identity. A nickname has no legal existence thus cannot have legal rights.
https://community.kde.org/Policies/Licensing_Policy states that commits must have commit author's names (and email).
But I am not too sure how much we would enforce this.
@papoteur
Do you realize your email is already pretty-much public on bugzilla and it contains your name ?
