[Nepomuk] Re: SimpleResource rcgen redesign draft

[Christian Mollekopf]

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.

> 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 =)
Further, just disable the copy constructor in the wrapper classes and all  
problems are gone (I reckon).

>> 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