SUMMARY On Krita/5.0 branch, importing a brush preset twice will cause an overwrite warning and the terminal will note that the MD5sum of the two presets do not match. STEPS TO REPRODUCE 1. Make a brush preset, copy it from `~/.local/share/krita/paintoppreset` to some local folder. 2. Close Krita, remove the krita configuration folder. (In my case, `~/.local/share/krita`) 3. Open Krita and import you brush; You will see that the import was successfull. 4. Reimport the same brush file. OBSERVED RESULT It will prompt you for an overwrite which is not correct as the MD5Sum and filename should be the same. Additionally, the terminal will read something like: ``` A resource with the same filename but a different MD5 already exists in the storage "paintoppresets" "/home/user/.local/tmp/c)_Pencil-2_Modification.kpp" "" Proceeding with overwriting the existing resource... ``` EXPECTED RESULT Importing should be bypassed, and the md5 sum should be the same.
Actually, this is also happening with palettes and patterns.
In that case, I'm tagging this as a release blocker. I can't change the importance for some reason, but...
There seems to be two problems: 1) The contents of two import functions KisResourceUserOperations::importResourceFileWithUserInput() and ResourceImporter::importResources() has diverged, so they do different things to check the resource deduplication. 2) Neither of them checks if exactly the same resource is present in the database. 3) I guess, in case the resource exists in the database, but **inactive** or **overriden by a new version**, then the user should also be asked about that. Though it will change the strings.
https://bugs.kde.org/show_bug.cgi?id=446705 seems related.
(In reply to Dmitry Kazakov from comment #3) > There seems to be two problems: > > 1) The contents of two import functions > KisResourceUserOperations::importResourceFileWithUserInput() and > ResourceImporter::importResources() has diverged, so they do different > things to check the resource deduplication. I guess this should be taken care of soon. It's strange because, during the last testing phase, I don't remember having an issue like this running through Tiar's testing parameters. Maybe I'll take a look through the importing resource pipeline today to see how easy this will be to resolve. > 3) I guess, in case the resource exists in the database, but **inactive** or **overriden by a new version**, then the user should also be asked about that. Though it will change the strings. That is an interesting point, and one that we won't be able to easily handle in Krita 5 since we're in a string freeze. In the case that something like this happens we should ask the user whether they want to revert a resource to the previous version, or change the "file name" to create a new branch of the resource itself.
dmitryk, Are you sure this isn't a bigger issue with the way the md5 sums work? ``` Entering "KisResourceLocator::importResource()" existingResource->filename() = "c)_Pencil-2_Copy.kpp" existingResource->md5Sum() = "065415ee40bbf674438590e85a31a643" resourceUrl = "paintoppresets/c)_Pencil-2_Copy.kpp" md5 = "0c415c37996944b9b14f91e1052b6481" ``` Seems like these MD5 sums should be the same but their not. The results of generateHash don't seem to equal the results of the resources md5sum function which is a bit bizarre.
Hi, Eoin! > Are you sure this isn't a bigger issue with the way the md5 sums work? I bet you are requesting MD5 via loadedResource->md5(), which (for a resource not loaded into any storage) triggers a fallback and generates MD5 on the fly. This MD5 is extremely fragile and cannot be used. You need to calculate MD5 explicitly on the resource file using `KoMD5Generator::generate()`.
Here is a bigger explanation of a problem: https://phabricator.kde.org/T14946
Ah, there is a bug in KisResourceLocator::importResource(), it calculates md5 sum on the fly... I'll looks into it as a part of bug 446743
Okay, I need to hijack this bug because it is tightly coupled with bug 446743. The fix will be in this MR: https://invent.kde.org/graphics/krita/-/merge_requests/1216
Git commit a4c243b1b0af33ca72e66d8819ba2bdf7486ac78 by Halla Rempt, on behalf of Dmitry Kazakov. Committed on 13/12/2021 at 15:51. Pushed by dkazakov into branch 'master'. Fix md5 calculation in KisResourceLocator::importResource() We should request MD5 from the storage instead of using the fallback in the resource itself. Related: bug 446743 Ref T14946 M +2 -1 libs/resources/KisResourceLocator.cpp https://invent.kde.org/graphics/krita/commit/a4c243b1b0af33ca72e66d8819ba2bdf7486ac78
Git commit 358bc9f1c0624a67565c0884120787140cfa7a9e by Halla Rempt, on behalf of Dmitry Kazakov. Committed on 13/12/2021 at 15:51. Pushed by dkazakov into branch 'master'. Add unittests for KisResourceLocator::importResource The requirements the tests check are the following: 1) When 'override' flag is not set we will never succeed importing, even when the existing resource is exactly the same. It is needed to avoid confusion in the models that expect the resource to be **added** for real. 2) When override flag is set and the exactly the same resource (with the same filename case, and with the latest version) exists, then we return this resource. 3) Otherwise we remove all the versions of the existing resource and add the new one. Related: bug 446743 M +36 -0 libs/resources/KisResourceCacheDb.cpp M +2 -0 libs/resources/KisResourceCacheDb.h M +59 -39 libs/resources/KisResourceLocator.cpp M +30 -0 libs/resources/tests/TestBundleStorage.cpp M +1 -0 libs/resources/tests/TestBundleStorage.h M +28 -0 libs/resources/tests/TestFolderStorage.cpp M +1 -0 libs/resources/tests/TestFolderStorage.h M +93 -0 libs/resources/tests/TestResourceLocator.cpp M +1 -0 libs/resources/tests/TestResourceLocator.h https://invent.kde.org/graphics/krita/commit/358bc9f1c0624a67565c0884120787140cfa7a9e
Git commit e373f57133f5935cead226567fa2ad4fab9b6bd1 by Halla Rempt, on behalf of Dmitry Kazakov. Committed on 13/12/2021 at 15:51. Pushed by dkazakov into branch 'master'. Fix importing the duplicated resources This patch also makes the import-overwrite code check consistent between the two places where we do that, in KisResourceUserOperations::importResourceFileWithUserInput() and in ResourceImporter::importResources(). Related: bug 446743 M +11 -0 libs/resources/KisResourceLocator.cpp M +9 -1 libs/resources/KisResourceLocator.h M +11 -0 libs/resources/KisResourceModel.cpp M +16 -0 libs/resources/KisResourceModel.h M +6 -0 libs/resources/KisTagFilterResourceProxyModel.cpp M +1 -0 libs/resources/KisTagFilterResourceProxyModel.h M +6 -0 libs/resources/KisTagResourceModel.cpp M +1 -0 libs/resources/KisTagResourceModel.h M +1 -9 libs/resourcewidgets/KisResourceUserOperations.cpp M +0 -1 libs/resourcewidgets/KisResourceUserOperations.h M +4 -2 plugins/extensions/resourcemanager/ResourceImporter.cpp https://invent.kde.org/graphics/krita/commit/e373f57133f5935cead226567fa2ad4fab9b6bd1
Git commit e4645fe2a6601b9ab60eb17c4f5ae02430b2c2d2 by Dmitry Kazakov. Committed on 13/12/2021 at 16:21. Pushed by dkazakov into branch 'krita/5.0'. Fix md5 calculation in KisResourceLocator::importResource() We should request MD5 from the storage instead of using the fallback in the resource itself. Related: bug 446743 Ref T14946 M +2 -1 libs/resources/KisResourceLocator.cpp https://invent.kde.org/graphics/krita/commit/e4645fe2a6601b9ab60eb17c4f5ae02430b2c2d2
Git commit 4bcf9f46b431b7785591458b42c5afcadc21c5e4 by Dmitry Kazakov. Committed on 13/12/2021 at 16:21. Pushed by dkazakov into branch 'krita/5.0'. Fix importing the duplicated resources This patch also makes the import-overwrite code check consistent between the two places where we do that, in KisResourceUserOperations::importResourceFileWithUserInput() and in ResourceImporter::importResources(). Related: bug 446743 M +11 -0 libs/resources/KisResourceLocator.cpp M +9 -1 libs/resources/KisResourceLocator.h M +11 -0 libs/resources/KisResourceModel.cpp M +16 -0 libs/resources/KisResourceModel.h M +6 -0 libs/resources/KisTagFilterResourceProxyModel.cpp M +1 -0 libs/resources/KisTagFilterResourceProxyModel.h M +6 -0 libs/resources/KisTagResourceModel.cpp M +1 -0 libs/resources/KisTagResourceModel.h M +1 -9 libs/resourcewidgets/KisResourceUserOperations.cpp M +0 -1 libs/resourcewidgets/KisResourceUserOperations.h M +4 -2 plugins/extensions/resourcemanager/ResourceImporter.cpp https://invent.kde.org/graphics/krita/commit/4bcf9f46b431b7785591458b42c5afcadc21c5e4
Git commit 00cfb9213d53cc1b4683c912dc6e46593e4dd14f by Dmitry Kazakov. Committed on 13/12/2021 at 16:21. Pushed by dkazakov into branch 'krita/5.0'. Add unittests for KisResourceLocator::importResource The requirements the tests check are the following: 1) When 'override' flag is not set we will never succeed importing, even when the existing resource is exactly the same. It is needed to avoid confusion in the models that expect the resource to be **added** for real. 2) When override flag is set and the exactly the same resource (with the same filename case, and with the latest version) exists, then we return this resource. 3) Otherwise we remove all the versions of the existing resource and add the new one. Related: bug 446743 M +36 -0 libs/resources/KisResourceCacheDb.cpp M +2 -0 libs/resources/KisResourceCacheDb.h M +59 -39 libs/resources/KisResourceLocator.cpp M +30 -0 libs/resources/tests/TestBundleStorage.cpp M +1 -0 libs/resources/tests/TestBundleStorage.h M +28 -0 libs/resources/tests/TestFolderStorage.cpp M +1 -0 libs/resources/tests/TestFolderStorage.h M +93 -0 libs/resources/tests/TestResourceLocator.cpp M +1 -0 libs/resources/tests/TestResourceLocator.h https://invent.kde.org/graphics/krita/commit/00cfb9213d53cc1b4683c912dc6e46593e4dd14f