[PyKDE] Implementation of KConfigSkeleton::addItemUInt

[Michael Franz Aigner]

On Wed, 1 Sep 2004 14:14:07 -0700, Jim Bublitz <jbublitz at nwinternet.com> wrote:
> This is what KDE does:
>
> KConfigSkeleton::ItemBool *KConfigSkeleton::addItemBool (onst QString &name,
>      bool &reference,  bool defaultValue, const QString &key )
> {
> KConfigSkeleton::ItemBool *item;
>  item =
>   new KConfigSkeleton::ItemBool( mCurrentGroup, key.isNull() ? name : key,
>                                                     reference,defaultValue );
>  addItem( item, name );
>  return item;
> }
>
> In Python you should be able to do:
>
>   self.reference = True
>   self.item = KConfigSkeleton.ItemBool (mCurrentGroup, name, reference,
>                          defaultValue)
>   self.addItem (self.item, name)
>
> I'm not sure I handled the key/name distinction correctly, but it should be
> doable. I'm also not sure if that fixes the "scalar-by-reference" problem
> either, but I think it does.
>

Would be nice, but i'm afraid, it doesn't work out. The KConfigSkeleton.ItemBool
constructor suffers from the same problem as KConfigSkeleton.addItemBool().
Let me explain. The following is the C++ code that sip generates for the
KConfigSkeleton.ItemBool constructor:

if (!sipCpp)
{
    QString * a0;
    int a0IsTemp = 0;
    QString * a1;
    int a1IsTemp = 0;
    bool a2;
    bool a3 = 1;

    if (sipParseArgs(&sipArgsParsed,sipArgs,"M1M1b|b",sipConvertTo_QString,&a0,&a0IsTemp,sipConvertTo_QString,&a1,&a1IsTemp,&a2,&a3))
    {
#line 355 "sip/kdecore/kconfigskeleton.sip"
//takes group | (QString) | key | (QString) | reference | (bool) |
defaultValue | (bool = 1)
    Py_BEGIN_ALLOW_THREADS
    sipCpp = (sipKConfigSkeleton_ItemBool *) new
KConfigSkeleton::ItemBool (*a0, *a1, a2, a3);
    Py_END_ALLOW_THREADS
#line 21908 "sipkdecorepart0.cpp"

        if (a0IsTemp)
                delete a0;

        if (a1IsTemp)
                delete a1;
    }
}

I used sip 4.0.1 on PyKDE 3.11.3, and i only pasted the important part of the
function here.

In the code generated by sip you can see that a local bool variable is passed
to KConfigSkeleton::ItemBool. Here lies the problem. The
KConfigSkeleton::ItemBool constructor should never be given a reference to a
local variable (i.e. on the stack).

Coming back to your example:
>   self.reference = True
>   self.item = KConfigSkeleton.ItemBool (mCurrentGroup, name, reference,
>                          defaultValue)

Passing a value of Python types like Bool, Int, ... to a function by reference
and letting the function change the value can not work out, because in Python,
those types (bool, ...) are by design immutable. Their value can not be changed.
It does not work in Python code, so IMHO it probably can not be translated into
C++ code either (by sip, i mean).
To summarize: I would wish your example would work, but it does not.

> In that case, you should be able to overload addItemBool in Python to take
> self.item instead of the bool reference. Seems to me that should work,
> although I obviously don't have the details worked out. I'd prefer that to
> adding the additional classes.  What I'd like to avoid is "special" PyKDE
> methods/classes if at all possible. In this case, the required classes
> *almost* exist already.

I absolutely understand that you avoid 'special' PyKDE stuff.

Rethinking my approach, considering your suggestions, i was able to come up
with something much simpler, which does not introduce new special classes (at
least not at the Python level, it still needs some helper classes in C++).
Here is the patch:
http://amfranz.com/pykde/PyKDE-3.11.3-kconfigskeleton.diff

It removes the 'reference' argument from the argument list of the
addItemInt/addItemBool/... functions. The functions will allocate the reference
variable internally, and in a way so it gets freed automatically along with the
returned KConfigSkeleton.Item* object.

However, afaik, the *.sip files are are generated by a tool called presip, so i
guess, so a permanent fix would have to be done in this tool. Am i correct in
this?

I also have a variant which works without declaring any new classes, by using
placement-new:
http://amfranz.com/pykde/PyKDE-3.11.3-kconfigskeleton-placement-new.diff

However, I am not so sure about it, i usually try to avoid placement-new. And
all this pointer juggling doesn't look like clean code either.

> I could also modify the addItem* calls to create the referenced scalar on the
> heap instead of as a temporary. I'm not sure how it would get deleted though
> - probably in the dtors for the Item* objects.  There are some other things I
> could try along those lines, including not calling the C++ addItem* methods
> at all, but doing the equivalent on the sip C++ code side if that proved
> useful.

> Would you be able to try out the Python suggestion above?
Did try, but doesn't work :(

> It's going to take me some time to work through this, as I don't have any
> working code that uses KConfigSkeleton, and whatever solution I try, I'd like
> to be able to test here. I also need to test out the Item<scalar> classes and
> verify that the Python code (including "casting") works correctly - those
> might also need some work. It's likely to take some time.
Mind if i help you a bit? I've made a small test case:
http://amfranz.com/pykde/kconfigskeleton-test/

It's a small application which displays a configuration dialog with some
settings to configure, all of which are remembered in
~/.kde/share/config/kconfigskeletontestrc


Off-topic question: I noticed that addItemInt64/addItemUInt64 are declared
three times in kconfigskeleton.sip, all inside different %If..%End blocks. Is
there a specific reason for that?