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

[Boudewijn Rempt]

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

Ship it!


Yes, two bugs... The first, with the '0' adding path to /usr/krita, that's a copy and paste error, and the weird line in LcmsEnginePlugin was added when Srikanth added GHNS support for profiles. It's totally wrong... As long as Krita also keeps looking in data krita/profiles, everything should be fine.

- Boudewijn Rempt


On April 11, 2015, 1:38 p.m., Friedrich W. H. Kossebau wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123331/
> -----------------------------------------------------------
> 
> (Updated April 11, 2015, 1:38 p.m.)
> 
> 
> 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
> 
>

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