KMetaData API comments

[Sebastian Trüg]

On Wednesday 02 May 2007 20:20:57 Simon Hausmann wrote:
> > > Since all of these getters are just convenience to prevent developers
> > > from having to write those long uris how about solving this problem
> > > using an enum instead? That would also reduce the public API
> > > dramatically and makes it really cheap to extend in the future. Right
> > > now when you want to add a few property you have to add two more
> > > exported symbols while an added enum does not affect the number of
> > > exported symbols.
> > >
> > > I'm thinking of something along the lines of this:
> > >
> > > Variant property(const QString &uri) const;
> > > void setProperty(const QString &uri, const Variant &value);
> > >
> > > enum PropertyType {
> > >     SymbolsProperty,
> > >     RatingProperty,
> > >     IdentifiersProperty,
> > >     IsRelatedProperty,
> > >     IsTopicOfProperty,
> > >     ...
> > > };
> > > Variant property(PropertyType type) const;
> > > void setProperty(PropertyType type, const Variant &value);
> > >
> > > On top of that such an enum based API is also easier to bind from other
> > > languages because not every setter/getter needs to be bound
> > > individually.
> >
> > This is a nice idea but would have to be thought through a little more
> > because the idea is to have all that stuff autogenerated from the
> > ontologies. In this case the Tag class is completely autogenerated to
> > handle Tags. So extensions are done by using the resource generator to
> > create new classes (see the doygen documentation).
>
> Ok, but you can still auto-generate the table that maps from the enum value
> to the uri, no?
>
> From an API perspective I don't see any real disadvantages. It's not that
> the class becomes harder to use, while on the other hand it reduces the
> total amount of functions, exports less symbols, makes it easier to bind
> from other languages and it solves the naming problem. And especially the
> latter seems quite important to me, with function names like
> Resource::isTopicOfOf(), getIsRelateds() and getIsTopicOfs().

I agree that the method names are quite ugly but not having the convenience 
classes means that you would have to create resources like this (the enum 
names are just an example and are ugly ;):

Resource myFile( pathToFile, KNepomuk_enum_File_Type );

And then to retrieve properties you would have to take care of the conversion:

int rating = myFile.property( KNepomuk_enum_Properties_rating ).value<int>();

Although this is not very complicated problems cannot be seen at compile time. 
If you use a slightly wrong type (unsigned instead of signed or whatever) 
your rating won't work. I just think that the generation of code is a good 
way of preventing errors and making code more readable.
As for the binding issue: you would not bind all the generated classes anyway. 
You would bind Resource and then maybe use a different code generator for the 
bindings. This is the same for your approach. You would have to generate the 
enumerations for the bindings also. So in terms of bindings I see no real 
advantage.

> > > The only other thing in the API that *seriously* worries me is the
> > > Variant class that duplicates the *entire* QVariant API and registers a
> > > whole lot of types with the QMetaType registry.
> > >
> > > Wouldn't it be simpler to solve the convenience problem this class
> > > tries to solve in the Resource API? You could have overloads for
> > > example:
> > >
> > > QVariant property(const QString &uri) const;
> > > QList<Resource> resourceListProperty(const QString &uri) const;
> >
> > Actually I think the Variant class makes a lot sense here and only may be
> > wrongly named. Sth along the lines of PropertyValue or simply Value would
> > probably be better. The thing is that KMetaData does not support all
> > types QVariant supports. And the second thing is that at some point I
> > will add additional data to the properties such as their creation and
> > modification date. I think an extra class makes even more sense then.
> > What do you think?
>
> I don't think the lack of support for all variant types is a problem at
> all. You can still spit out a warning at run-time for example. But your

Warnings at runtime are awful. They don't help the user at all and tracking 
them down as a developer is always a pain.
Again I have to play the "find errors at compile time" card. :)

> point of adding extra fields is a good one which makes me think that
> perhaps your ProperyValue class should /have/ a QVariant /next/ to the
> other properties instead of /being/ a QVariant. With an implicit
> constructor it should still be convenient enough to use, but at the same
> time it allows you to get rid of all the API that is duplicated from
> QVariant. And it gives you more flexibility in the future for extensions
> (while public inheritance from QVariant limits you quite a bit).

I agree that we could remove the inheritance from QVariant and use a member. 
But I would still like to keep the API because it is very convenient and I 
don't see why we would have to simplify the API just to export less symbols 
while we make using it harder. In the end each app will implement these 
conversion stuff themselves so what's the point?

Cheers,
Sebastian