KMetaData API comments

[Simon Hausmann]

On Wednesday 02 May 2007 13:29:00 Sebastian Trüg wrote:
[...]
> > Resource::revive()
> >
> > 	In line with markForDeletion() this function could be called undelete().
> > That I think makes the relation to markForDeletion() also clearer (it is
> > not obvious that revive relates to remove IMHO).
>
> undelete sounds a lot like delete which is confusing IMHO. Maybe this
> method could better be hidden from the public API since I am unsure if this
> will ever really be needed (if in doubt leave it out)

Yup, leaving it out sounds like the best option then.

> > Resource::isProperty(const QString &uri) const
> >
> > 	Since this function is very different from the other property API in
> > this class I suggest to disambiguate it by choosing a different name, if
> > it is useful at all for applications. Perhaps the one line that it
> > substitutes is not worth the naming conflict? I suggest to remove it
> > alltogether. The call to allResourcesWithProperty is easier to read IMHO
> > despite its length.
>
> You are right, it is not well named. Maybe something
> like "isUsedAsValueForProperty"...
[...]

> > bool Resource::inSync() const
> >
> > 	Since this relates to modified but explains the status towards the
> > metadata store, how about calling it isModifiedInStore()? Or maybe
> > isModifiedOnDisk() would be more illustrative. (of course that would also
> > reverse the meaning, but I think it's worth it).
>
> maybe inSyncWithStore because modifiedInStore suggests that a local
> modification is not taken into account which it actually is.

Good point, I like inSyncWithStore.

[...]
> > 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().

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

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/20070502/ce30b5d8/attachment.sig>