Skip to content

WIP: Fix resource models going crazy with indices

Agata Cacko requested to merge tymond/krita:tiar/two_resource_models_and_palettes_issues into master

What I did

  1. I tracked down the commit responsible for the palette not switching to the palette currently created. The commit is this: fb1d9bad
  2. Added a unit test, conclusions below.

Conclusions

  1. Commit fb1d9bad is probably ok, I just probably made it before Dmitry's changes (0b260f71) and rebased without checking if it works afterwards. After fixing the indexing issues, one should check if it works better with or without this commit.

  2. The unit test I added (b19446d6, "Add unit test for two resource models") has those results:

    • passes if you run only that test (shows that QSortFilterProxy works as expected and get the info)
    • passes if you call invalidate() to the other model after adding the resource (but that's cheating, we cannot really use it in the code reliably, and hides the actual issues, proxy models should have it figured out on their own)
    • passes if you change the dummy.kpp to dummy_temp.kpp in addTemporaryResources() (change_name_in_addTemporaryResources_for_unittest_to_pass.diff) - this confirms it's an issue caused by GROUPBY statement (otherwise it wouldn't matter whether the name is unique or not).
    • fails otherwise

  3. Both resource models have the same sourceModel() set up, and the QSignalSpy does see the signal, which both proxy models probably got. So the problem is somewhere else, not in having two resource models per se.

  4. If you use the debug added in the commit b67424cf ("Add debug to two resource..."), you'll see that the reason there are some weirdness in the indexing is that after addTemporaryResources() call adding a resource with non-unique resource, the source model gets a new index, but everything that index points to is an invalid variant.

    QSYSTEM: TestResourceModel::testTwoExistingResourceModels() Resource name: QVariant(QString, "dummy.kpp") for index 4

    QSYSTEM: TestResourceModel::testTwoExistingResourceModels() Resource name: QVariant(Invalid) for index 5

  5. Since I noticed that we could actually call "beginInsertRows" etc. after we add it to the database in the Locator, so we know if it succeeds or not, I tried to fix the KisResourceModel to always use it correctly. However, it didn't change anything since I think in the unit tests all operations are succcessful, so there is no issue in assuming it will succeed. However it would be a good idea to fix it anyway, but one must be careful in implementing it and making sure it works afterwards. A draft for that is here: fixes_for_resource_model_to_only_send_a_signal_when_needed.diff

What needs to be done

  1. Figure out why indexing breaks with the current GROUPBY-based statement.
  2. Fix it. (I personally would prefer a solution where KisAllResourcesModel really do show all resources and only KisResourceModel have them de-duplicated, or the deactivated are hidden etc. I thought that was the point of that class. Smart hiding anything means that you no longer can rely on that class to tell you a reliable answer to "does this thing exist there" or "find me this thing". But any working solution will be fine, I guess).
  3. Tests in at least two or three different resource types. Preferably something simple and straight-forward as well as something less simple.
  4. Test palette docker, especially creation and autosaving on exit. Bug 444309 (https://bugs.kde.org/show_bug.cgi?id=444309) should be fixed automatically.
  5. Rebase this MR over latest master; note that this is before Wolthera's changes to isModified to use isDirty instead.
  6. Make sure you don't lose smaller fixes I added to this MR regarding palettes.

Test Plan

  1. Make sure the unit test with two resource models passes even when run with all other tests.
  2. Make sure the palettes can be added and work fine.
  3. Find at least two other resource types and check all the possible operations are successful.
  4. Make sure that for example if you add a new pattern, you can see that newly added pattern in the patterns docker.

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 Halla Rempt

Merge request reports