KMetaData API comments

[Simon Hausmann]

On Friday 04 May 2007 11:33:43 Sebastian Trüg wrote:
> 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.

Actually the existing binding generators can cope with enums just fine since 
we use them a lot in KDE/Qt APIs after all. And with G_GADGET it's trivial to 
write bindings "by hand" by letting moc do the string/value mappings.

But if you really insist on the getter/setter functions pretty please with a 
cherry on top reconsider names like getIsTopicOfs(), getIsRelateds() or 
isTopicOfOf().

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

I don't think you can catch the errors at compile time at the moment:

QImage image = ...
resource.setProperty(..., image);

That I would say is very likely to compile, as long as your QVariant 
constructor is implicit.

On top of that QVariant is not really designed to be publically subclassed in 
the first place because it does not have a virtual destructor. If somebody in 
their application deletes (using "delete") a QVariant object that is actually 
a Nepomuk::KMetaData::Variant then they trigger undefined behavior.

(and we're not going to make QVariant's destructor virtual, we cannot do that 
anyway without breaking binary compatibility)

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

I don't see why you cannot have the overloads that get/set your list types in 
Resource itself?

Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20070506/e64ba434/attachment.sig>