-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123331/
-----------------------------------------------------------
Review request for Calligra, Cyrille Berger Skott, Boudewijn Rempt, and
Srikanth Tiyyagura.
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
_______________________________________________
calligra-devel mailing list
calligra-devel@kde.org
https://mail.kde.org/mailman/listinfo/calligra-devel
