Skip to content

Resources: WithUserInput functions improvements

What I did

  • added one null check
  • moved creation of KisResourceModel to inside of the function because:
    • most of the time the model had to be created new anyway
    • even if it wasn't, since it's a filter proxy model, we have no guarantee that it will contain all available resources, and especially it doesn't have deleted resources, so it would be better to just use a fresh new model with the filtering that the function needs

TODO

  • Make the addResource capable of overwriting
  • Make custom_pattern just use addResource and not use that QFileDialog - disputable
  • Make importResourceFile a bit more safe by using that function from cacheDb, I guess? Though what if it doesn't notice the file that is there but not in the database? But that's not my problem, isn't it? - disputable, probably better leave it as it is
  • make it more sane for resources that are deleted (maybe ask, but add a note that the resource is deleted anyway). - not necessary

Logic in functions

This picture shows the logic flow of the addResourceWithUserInput. There are three different outcomes: add, overwrite (which means, create a new resource) and cancel (don't do anything). The graph doesn't show it but there is also an error message every time there is a failure (I mean after a failure the operation is cancelled, so an error message can only be shown once). Legend:

  • the purple diamonds are user dialogs
  • the green ones are determined in code without user interaction
  • filename clash = a resource with the same filename in the same storage in the same resource type (this is just not really allowed so I want to catch it so the user can react to it, and not just see an error message)
  • name clash = a resource with the same name in the same resource type (this is just for user sanity, not everyone would like to have fifteen dogs all called "Jack", y'know).

NOTE: There is one change necessary in this graph, basically, now you can't go from name clash to overwrite because there can be multiple resources called the same way so it's not obvious which resource to overwrite. Could be enhanced in the future, but for now, let's just leave it out.

addResourceWithUserInput

Logic in other functions should be similar.

For example in renameResourceWithUserInput, there is no filename clash, but a name clash can happen so I try to catch it and again, provide the same options: rename (instead of add), overwrite and cancel.

In overwriteResource, the filename clash again cannot happen, but the name clash can happen (if the user changed the name as well). Currently in this case I just offer overwriting the resource that matches the filename and resource Id; I could, in theory, also offer overwriting the resource that matches only the name, but I think it would be too confusing for the user. The other option is cancel, of course.

In importResourceFileWithUserInput the current situation is different. It checks if the filename clashes, and then asks the user, as usual. Except that this time if the user says "yes", the old resource is completely wiped out of resource folder and the new resource file is imported.

How to discover filename clash

There are several methods to discover filename clash.

  • KisResourceModel::resourcesForFilename() - this one is not good for this purpose because it only contains resources that have current version filename equal to the filename I asked for. So if there are already two versions of "Laaa" (with base filename Laaa.pat) resource, if I try to add a resource with filename Laaa.pat, it will return that it's fine because the current filename for the Laaa resource would be Laaa.0001.pat. (I don't count not showing deactivated resources by default because I can set it up to show them).
  • KisResourceCacheDb::getResourceIdFromVersionedFilename(filename, resourceType, storageLocation, outResourceId) - this one is good because it gets filenames from all versions of all resources. However it doesn't show files that are in a storage but aren't present in the database.
  • KisResourceUserOperations::resourceExistsInResourceFolder(resourceType, filename) - this obviously only works for folder storage.

Which one should I use? I think KisResourceCacheDb::getResourceIdFromVersionedFilename but maybe I should use both in case the resource wants to be added/imported to the folder storage, just to be extra safe?

Test Plan

  1. Delete a resource
  2. Create a new resource with the same name as the deleted resource
  3. Should be no crash!

Formalities Checklist

  • I confirmed this builds.
  • I confirmed Krita ran and the relevant functions work.
  • I tested the relevant unit tests and can confirm they are not broken. (If not possible, don't hesitate to ask for help!)
  • I made sure my commits build individually and have good descriptions as per KDE guidelines.
  • I made sure my code conforms to the standards set in the HACKING file.
  • I can confirm the code is licensed and attributed appropriately, and that unattributed code is mine, as per KDE Licensing Policy.
Edited by Agata Cacko

Merge request reports