KMetaData API comments

[Sebastian Trüg]

On Sunday 06 May 2007 18:15:57 Simon Hausmann wrote:
> 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.

I am sure enums can be handled easily but you cannot bind to all enums at the 
same time which means you have to generate new bindings for each new type 
anyway. So you can do whatever you like in the bindings, for example just 
binding to the get/set property methods.

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

These are ugly. The problem is that the method names are generated from the 
ontology properties. But I will see if I can improve them. :)

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

Hmm, you are right. So to make my argument stick the QVariant constructor 
would have to be made expicit...

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

Right. I did not think of that. So I will remove the QVariant subclassing and 
use a member instead.

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

Because it is cleaner this way. The resource is a container for properties and 
a property can be a list of things or a simple thing.
Additionally if we add provedance data in the future we would have to put that 
into Resource again, making it even more complicated. With an additional 
Variant (or better: PropertyValue) class the API is just cleaner.

Cheers,
Sebastian