KMetaData API comments

[Sebastian Trüg]

Sorry for the late reply. I had no email connection.

On Tuesday 01 May 2007 21:44:48 Simon Hausmann wrote:
> I've just had a look at the kmetadata public API and I have a few
> suggestions regarding consistency and naming. Please consider them
> constructive criticism.
>
> QString KMetaData::errorString(int code);
>
> 	I think it would be nice of this function would take an enum instead.

agreed.

> enum ErrorCode {
>            ERROR_SUCCESS = 0,
>            ERROR_COMMUNICATION,
>            ERROR_INVALID_TYPE
> };
>
> 	In Qt/KDE enum values usually have camel case names instead of all upper
> case. The no-error value is usually also called "NoError" instead of
> having "success" in the name. (NoFoo is the common pattern).
>
> I suggest
>
> enum ErrorCode {
>     NoError,
>     CommunicationError,
>     InvalidTypeError
> };
>
> int Resource::sync()
>
> 	I guess this function should return a ErrorCode enum instead if a plain
> int.

right.

> Variant Resource::getProperty(const QString &uri) const;
>
> 	Getters usually don't have a get suffix in Qt/KDE. I suggest to just call
> it "property".

fine by me. :)

> Resource::remove()
>
> 	The documentation explains that this doesn't actually delete the resource
> but only marks it for deletion. I suggest to call it markForDeletion().

Well, actually it would be better to improve the documentation then because it 
will be deleted if auto-syncing is enabled.

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

> 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::modified() const
>
> 	The getter for a "modified" property is usually called "isModified" in
> Qt/KDE. I suggest to add the "is" prefix.

ok.

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

> QStringList Resource::getSymbols() const
> quint32 Resource::getRating() const
> QStringList Resource::getIdentifiers() const
> QStringList Resource::getPrefLabels() const
> QStringList Resource::getLabels() const
> QList<Resource> Resource::getAnnotations() const
> QString Resource::getComment() const
> QList<Tag> Resource::getTags() const
> QList<Resource> Resource::getTopics() const
>
> 	I think all of these getter functions should not have the "get" prefix.

right. these are auto-generated methods btw.

> QList<Resource> Resource::getIsRelateds() const;
> QList<Resource> Resource::isTopicOfOf() const;
>
> 	Those feels like very very unusual names.

again auto-generated from the NAO ontology.

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

> Perhaps this would also make it possible to get rid of the Tag class?

see above.

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

> We do the same in Qt with QTextFormat for example, so it's not an entirely
> new pattern, and I think it is much better than duplicating the entire
> QVariant API, which is just asking for maintenance trouble IMHO.
>
> Again, please consider this constructive criticism. I've only mentioned the
> bits that look inconsistent to me. There are indeed a lot of names that
> IMHO are very well chosen, for example ResourceManager::allResourcesOfType
> to name :) one.

thanks a lot for your input. I will work on these for next monday. :)

Cheers,
Sebastian