Review Request 123331: Cleanup dirs where Krita searches for ICC profiles

[Friedrich W. H. Kossebau]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123331/
-----------------------------------------------------------

(Updated April 13, 2015, 8:11 p.m.)


Status
------

This change has been marked as submitted.


Review request for Calligra, Cyrille Berger Skott, Boudewijn Rempt, and Srikanth Tiyyagura.


Changes
-------

Submitted with commit 462743a9cd6ca48597d039815806aae100050afd by Friedrich W. H. Kossebau to branch calligra/2.9.


Repository: calligra


Description
-------

While getting an overview over all the KStandardDirs usage in Calligra, to get an idea what is needed with KF5 where there is a bigger API/functionality breakage, I saw some issues with how ICC profiles are searched for.

Let's see if I have got things correctly:
ICC profiles are installed by different packages in `*/share/color/icc/*`. E.g. on my system there are number of different folders below `/usr/share/color/icc/`.
That is why the LcmsEnginePlugin calls
```
    KGlobal::mainComponent().dirs()->addResourceType("icc_profiles", 0, "share/color/icc/");
```
to get any such paths below the usual system prefixes (second argument `0` means immediate concatenation, without further grouping prefixes).
Krita itselfs installs all its own set of profiles in `${CMAKE_INSTALL_PREFIX}/share/color/icc/krita`.
Krita's ColorSettingsTab::installProfile() supports installation of additional ICC profiles, just to be used by Krita. It searches for the location to save the imported file with
```
    QString saveLocation = KGlobal::mainComponent().dirs()->saveLocation("icc_profiles");
```
which will give the first path in the list of prefixes + registered path suffix which can be written to. Which for me is `$KDEHOME/share/color/icc`.

So far things seem in order.


But then there are two calls that confuse me:

First is in KisFactory::componentData():
```
        s_componentData->dirs()->addResourceType("icc_profiles", 0, "krita/profiles/");
```
which will resolve to e.g. `/usr/krita/profiles/` or `$KDEHOME/krita/profiles/`.

Two questions:
a) Is that path resolution really wanted? Or should that have been done with `"data"` as second argument, so it would be resolved to e.g. `/usr/share/apps/krita/profiles/` or `$KDEHOME/share/apps/krita/profiles/`?
b) why that extra path, who is expected to put ICC files also in such folders?

Second issue is in LcmsEnginePlugin constructor:
```
    KGlobal::mainComponent().dirs()->addResourceDir("icc_profiles", QDir::homePath() + QString("/.kde/share/apps/krita/profiles/"));
```
Hardcoding `.kde` will not work on systems where the user data is stored under a different folder, after all that is configurable and possibly even platform-specific. Also giving a full path should not be needed.
This call seems to be a workaround for the first issue, as somebody has expected that ICC files in `$KDEHOME/share/apps/krita/profiles/` will be found.

So I propose to clean this up, by removing the code from LcmsEnginePlugin for registering the hardcoded path to the user's profile files.

And to either fix the registration of `krita/profiles/` or perhaps to even completely remove it (which I favour, unless there is a real need).

WRT to backwards compatibility, as `$KDEHOME/share/color/icc` seems to be picked first for saving ICC files via `saveLocation()`, removing both questionable resource path registrations should then only affect people who installed any files manually in the wrong `krita/profiles/` dirs or that hardcoded dir. Which I guess are not that many. And if they are, they surely are also able to fix it for them.

So for clean code and not collecting too much someone-on-the-earth-might-be-unhappy-to-have-to-adapt boilerplate I would remove this all, and make a comment in the release notes :)
But not my call, so asking you as maintainer :)
If you want to keep it, I will add at least some "this is broken, remove in a sane moment" comment explaining what brokeness we have here.


Diffs
-----

  krita/ui/kis_factory2.cc 2f5ba41 
  plugins/colorengines/lcms2/LcmsEnginePlugin.cpp c73ecf6 

Diff: https://git.reviewboard.kde.org/r/123331/diff/


Testing
-------


Thanks,

Friedrich W. H. Kossebau

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20150413/42a4fed1/attachment.htm>