[Nepomuk] Re: SimpleResource rcgen redesign draft

[Sebastian Trüg]

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.

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.

> 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. ;)

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
>> _______________________________________________
>> Nepomuk mailing list
>> Nepomuk at kde.org
>> https://mail.kde.org/mailman/listinfo/nepomuk
> 
>