Fixed bug in KisPerChannelFilter that prevented it from loading/saving bookmarks v.3

[Dmitry Kazakov]

>
>  > The first issue is toXML/toLegacyXML stuff. These functions are
> implemented
> > in _different_ classes although they do the same things. More than that,
> > both of them are called from different parts of krita: bookmark manager
> > uses toXML, kis_kra_ uses toLegacyXML Why not have the only one? The only
> > united interface?
> .kra file are expected to use the toLegacyXML because this functions saves
> to an XML formated for the 1.6 .kra files. While toXML use the OpenRaster
> style, described in
> http://create.freedesktop.org/wiki/index.php?title=OpenRaster/Layers_Stack_Specification.
>
>
Do you mean filter's configuration should look like that?

<filter name="perchannel" type="applications:Krita:PerChannel">
     <params version=1>
       <param name="CurvesNumber">3</param>
       <param name="curve0">0,0;0.5,0.5;1,1;</param>
       <param name="curve1">0,0;1,1;</param>
       <param name="curve2">0,0;1,1;</param>
     </params>
</filter>


 even if it will be very inspired by the SVG spec.
>

I'm afraid we can't be inspired by SVGspec here at all. The point is they
suggest us to use linear interpolation for the curve (i guess this type of
the curve had been implemented in KCurve before my patch) . Maybe this type
is a good idea for some ideal theoretical curve as it's quite simple and
fast, but for real users (who are "inspired" by real professional
instrumens, such as PS, PictureWindowPro, TheGimp) it's at least _strange_
curve and almost unusable, because they are used to cubic splines.

Of course, we can interpolate cubic spline, drawn by the user, to the linear
one and write the latter to the file in SVG format. But the user won't be
able to change the curve later.

And the last issue against SVG. SVG's feComponentTransferis is stuck to RGB.
RGB is not the only colorspace in the world. ;)



> > KisBaseProcessor::defaultConfiguration(const KisPaintDeviceSP pd)
>
> The documentation says all.
>

Where can i take a look on this documentation?


> It's the configuration that will be used in the filter dialog when the user
> start the dialog.
>
> You forgot that way as well:
> KisFilterConfiguration * KisBaseProcessor::factoryConfiguration(const
> KisPaintDeviceSP) const
> whose name sucks and is a bit confusing (if anyone has a better idea,
>

createConfiguration?



> Filter configurations are saved in KisBookmarkedConfigurationManager which
> is a generic class which can be used with all kind of
> KisSerializableConfiguration to store them, this can be filters/generators
> configuration, paintop settings, ... The only way
> KisBookmarkedConfigurationManager can create the correct subclass of
> KisSerializableConfiguration is by using the KisFilterConfigurationFactory.
>
>
The problem is that KisKraLoader::loadAdjustmentLayer calls
KisBaseProcessor::defaultConfiguration(...). The latter one calls
factoryConfiguration and (if factoryConfiguration isn't overridden in
KisPerChannelFilter) creates KisFilterConfiguration instead of needed
KisPerChannelFilterConfiguration. This causes adjustment layers not being
loaded properly from .kra.

The easiest (and the most illogical) workaround is to change
KisBookmarkedConfigurationManager::defaultConfiguration, that has access to
filter's configurationFactory, to call factory's createDefault if nothing
can be loaded from kritarc.
KisSerializableConfiguration*
KisBookmarkedConfigurationManager::defaultConfiguration() const
{
    if (exists(KisBookmarkedConfigurationManager::ConfigDefault.id())) {
        return load(KisBookmarkedConfigurationManager::ConfigDefault.id());
    }
///skipped///
    return d->configFactory->createDefault();
}

In this case we won't need factoryDefault at all. But this change is _very
bad idea_ as it's illogical.

In my opinion Factories should somehow call Filter's defaultConfiguration
instead of having their own createDefault and create(...). In this case,
maybe, we won't even be needed to override KisConfigurationFactory for every
filter.


The difference between createDefault and create, is that create is used to
> unserialized configuration, and createDefault is used to create the first to
> use default configuration (now that I have said that, I note that there is a
> bug in the implementation, since it means
> KisBaseProcessor::factoryConfiguration is never called, and I think that
> KisFilterConfigurationFactory::createDefault should call
> KisBaseProcessor::factoryConfiguration).
>
>
I've just said above that KisBaseProcessor::factoryConfiguration *IS called*
in real life. And the result of such a call is dramatical.


-- 
Dmitry Kazakov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kimageshop/attachments/20090309/0fa83403/attachment.htm