[Nepomuk] Re: SimpleResource rcgen redesign draft

Sebastian Trüg trueg at kde.org
Wed Jul 27 08:10:38 CEST 2011 

On 07/27/2011 01:11 AM, Christian Mollekopf wrote:
> On Tue, 26 Jul 2011 20:57:46 +0200, Sebastian Trüg <trueg at kde.org> wrote:
> 
>> On 07/26/2011 05:06 PM, Christian Mollekopf wrote:
>>> On Tue, 26 Jul 2011 13:26:20 +0200, Sebastian Trüg <trueg at kde.org>  
>>> wrote:
>>>
>>>> On 07/25/2011 09:43 PM, Christian Mollekopf wrote:
>>>>> On Mon, 25 Jul 2011 21:20:45 +0200, Sebastian Trüg <trueg at kde.org>
>>>>> wrote:
>>>>>
>>>>>> On 07/25/2011 07:52 PM, Christian Mollekopf wrote:
>>>>>>> On Mon, 25 Jul 2011 18:30:05 +0200, Sebastian Trüg <trueg at kde.org>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> Now that the SimpleResource rcgen is in git master[1] some people
>>>>>>>> used
>>>>>>>> it (mostly for Akonadi feeders and the web extractor framework) and
>>>>>>>> found it to be a bit strange.
>>>>>>>>
>>>>>>>> This is true. Current usage is as follows:
>>>>>>>>
>>>>>>>> SimpleResource r;
>>>>>>>> NCO::PersonContact(&r).setFullname("foobar");
>>>>>>>>
>>>>>>>> The reason for this is simple: due to multi-inheritance problems I
>>>>>>>> cannot derive the generated classes from SimpleResource. Thus, we
>>>>>>>> need
>>>>>>>> wrappers.
>>>>>>>>
>>>>>>>> Now some ideas were floating around and I would like to settle on  
>>>>>>>> one
>>>>>>>> better solution.
>>>>>>>>
>>>>>>>> One idea would be to have a hybrid thing where you could do this:
>>>>>>>>
>>>>>>>>   NCO::PersonContact pc;
>>>>>>>>   pc.setFullname("foobar");
>>>>>>>>   SimpleResource r = rc.resource();
>>>>>>>>
>>>>>>>> and this:
>>>>>>>>
>>>>>>>>   SimpleResource r;
>>>>>>>>   NCO::PersonContact pc(r);
>>>>>>>>   pc.setFullname("foobar");
>>>>>>>>   r = pc.resource();
>>>>>>>>
>>>>>>>> (The last line would be required since all SimpleResources would be
>>>>>>>> copies.)
>>>>>>>
>>>>>>> Why can't you just take the reference to r, and the last copy  
>>>>>>> wouldn't
>>>>>>> be
>>>>>>> needed?
>>>>>>> Of course the accessor is still needed for the NCO::PersonContact  
>>>>>>> pc;
>>>>>>> usecase.
>>>>>>
>>>>>> I cannot use refs for security reasons. Imagine someone returns a
>>>>>> generated class from a method after putting in a ref to a local
>>>>>> SimpleRes. Anyway, SimpleRes is implicitly shared.
>>>>>
>>>>> Thats the same as with pointers, and it's c++ after all, developers  
>>>>> have
>>>>> to think a bit.
>>>>> I just think that forgetting r = pc.resource() is more likely than
>>>>> returning a generated class
>>>>> containing a local reference, because that is a problem you should be
>>>>> used
>>>>> to as c++ developer
>>>>> (although a bit nasty I admit) and
>>>>>
>>>>> SimpleResource r;
>>>>> NCO::PersonContact pc(r);
>>>>> pc.setFullname("foobar");
>>>>>
>>>>> doesn't really look "wrong" at all (unless you have a close look at  
>>>>> the
>>>>> header).
>>>>
>>>> Actually I think this only looks quite correct to you since you have
>>>> been using the old rcgen. If you look at pretty much all other Qt/KDE
>>>> APIs you will see that this is a typical copy situation.
>>>>
>>>
>>> Except if you think the class is a wrapper.
>>> The copying seems to me more java style.
>>
>> Well, the problem is that we want wrapping and a member SimpleResource...
>>
>>>> And I would really prefer not to use references. Aside from the
>>>> unwritten policy in Qt and KDE code to not do it it is so very error
>>>> prone since people tend to overlook the little "&". That is also why I
>>>> chose to use a pointer in the original version. It forces one to add  
>>>> the
>>>> "&" ones self and, thus, think about what one it doing.
>>>>
>>>
>>> Ok, my intuition is different, also before using the old rcgen.
>>> Also I happen to like references =)
>>
>> They are very powerful indeed.
>>
>>> Further, just disable the copy constructor in the wrapper classes and  
>>> all
>>> problems are gone (I reckon).
>>
>> We could do that but that would not solve the problem with the
>> reference. We want to allow both
>>   SimpleResource res;
>>   Contact c(res);
>> and
>>   Contact c;
>>   c.resource();
>> That means that we have to store a pointer and convert the reference to
>> a pointer (ugly!) since a reference member needs to be initialized in
>> the constructor. Thus, the second case would mean to create a
>> SimpleResource on the heap which leads to memory management trouble.
> 
> Ok, you're right, I completely forgot about the second usecase =)

Sadly I realized something yesterday evening: we cannot go this way as
it is not possible to keep a SimpleResource copy in the wrapper class.
If it was possible we could just as well derive from SimpleResource. But
the whole multi-inheritance problem applies here, too.
Thus, all that is left is a better type handling which I will do as
discussed and maybe a port to using refs instead of pointers because it
is nicer to use that way.

Cheers,
Sebastian

>>
>> Cheers,
>> Sebastian
>>
>>>>> Anyways, no strong preference from my side.
>>>>
>>>> So not get me wrong: I like the idea of making it fast. But I am also
>>>> very concerned about usability of the API. I would rather have it be
>>>> pretty than super-fast. And in the end the overhead of copying a few
>>>> pointers or primitives is nothing compared to the actual calling of
>>>> storeResources. ;)
>>>
>>> Agreed, it's not really a performance issue.
>>> I just don't like that last "r = pc.resource();", which is imo
>>> unnecessary, error prone and
>>> feels to me like java (where you never modify the object itself but  
>>> always
>>> get a copy back).
>>>
>>> Anyways, it's really just my personal opinion/preference, and I'm fine
>>> with either way.
>>> Especially since you did a good job in providing a nice api so far =)
>>>
>>> Cheers,
>>> Christian
>>>
>>>>
>>>> Cheers,
>>>> Sebastian
>>>>
>>>>> Cheers
>>>>>
>>>>>>
>>>>>>>>
>>>>>>>> We could go on like so:
>>>>>>>>
>>>>>>>>   r.addProperty(...);
>>>>>>>>   pc = r;
>>>>>>>>   pc.setResource(r);
>>>>>>>>
>>>>>>>> The only disadvantage I see here might be a slight performance
>>>>>>>> overhead
>>>>>>>> due to the copying of the resources.
>>>>>>>>
>>>>>>>> Opinions?
>>>>>>>
>>>>>>> Additionally it might be nice to have an accessor to the uri of the
>>>>>>> SimpleResource directly in the wrapper,
>>>>>>> and the << operator for the graph.
>>>>>>>
>>>>>>> So we would go from this:
>>>>>>>
>>>>>>>    Nepomuk::SimpleResource iconRes;
>>>>>>>    Nepomuk::NAO::FreeDesktopIcon icon(&iconRes);
>>>>>>>    icon.setIconNames( QStringList() << iconName );
>>>>>>>    graph << iconRes;
>>>>>>>    res.setProperty( Soprano::Vocabulary::NAO::prefSymbol(),
>>>>>>> iconRes.uri() );
>>>>>>>
>>>>>>> to this:
>>>>>>>
>>>>>>>    Nepomuk::NAO::FreeDesktopIcon icon;
>>>>>>>    icon.setIconNames( QStringList() << iconName );
>>>>>>>    graph << icon;
>>>>>>>    res.setProperty( Soprano::Vocabulary::NAO::prefSymbol(),  
>>>>>>> icon.uri()
>>>>>>> );
>>>>>>
>>>>>> nice.
>>>>>>
>>>>>> Cheers
>>>>>> Sebastian
>>>>>>
>>>>>>> Cheers,
>>>>>>>
>>>>>>> Christian
>>>>>>>
>>>>>>>>
>>>>>>>> Cheers,
>>>>>>>> Sebastian
>>>>>>>>
>>>>>>>> [1]
>>>>>>>> http://quickgit.kde.org/?p=kde-runtime.git&a=blob&h=304db56a9aff5523a5672415550c49d4b7269c3c&hb=344806a2e378f3421399cad39a63f14a4428308b&f=nepomuk/services/storage/lib/nepomuk-simpleresource-rcgen.py
>>> _______________________________________________
>>> Nepomuk mailing list
>>> Nepomuk at kde.org
>>> https://mail.kde.org/mailman/listinfo/nepomuk
>> _______________________________________________
>> Nepomuk mailing list
>> Nepomuk at kde.org
>> https://mail.kde.org/mailman/listinfo/nepomuk
> 
>

More information about the Nepomuk mailing list