<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta content="text/html;charset=ISO-8859-1" http-equiv="Content-Type">
</head>
<body bgcolor="#ffffff" text="#000000">
Aaron J. Seigo a écrit :
<blockquote cite="mid:200809130955.07604.aseigo@kde.org" type="cite">
<pre wrap="">On Saturday 13 September 2008, Adrien BUSTANY wrote:
</pre>
<blockquote type="cite">
<pre wrap="">But then what happens for people who want to compile things in the
playground which use AnnotationPlugins ?
</pre>
</blockquote>
<pre wrap=""><!---->
... they should get the code from kdelibs?
</pre>
<blockquote type="cite">
<pre wrap="">Just some detail about the infrastructure of the annotation plugins :
The "final user" (ie the developper using the plugins) uses a factory
classes, which will return plugins. Plugins all share a common
interface, but they don't install any header, you cannot instantiate
them directly. So you can't just move the File AnnotationPlugin to the
dialog tree, you need the whole code (and even if you could, the
resource inspector needs it).
</pre>
</blockquote>
<pre wrap=""><!---->
yes, the consumer needs the factory class header, the plugin needs the plugin
class header ... it would probably be safer to just require people to have a
copy of those headers around somewhere (e.g. "manually" installed; it's two
files, not hard ;)
</pre>
</blockquote>
Yep, a bit more than two (you need AnnotationProperty and
AnnotationResult and AnnotationResource, but it's not huge). I wonder
if we could automate that installation as a "pre-install" step before
installing the kde-nepomuk playground...<br>
<blockquote cite="mid:200809130955.07604.aseigo@kde.org" type="cite">
<pre wrap="">
looking at the classes, the factory class looks quite straightforward and like
it could be ready for kdelibs in very little time. the plugin class looks like
it could use some api review and has greater possibility for BIC changes as it
has quite a few virtuals.
looking at the plugin class, some things that came to mind:
* isReady / name / iconName are all virtual. this means that if you ever want
to add some shared logic to isReady, for instance, you can't. it also means
every plugin needs to have at least three lines in their header and 6 lines in
the implementation. if you provided protected setters and made the getters
non-virtual, not only can you add shared logic easily later on (and reduce the
size of the virtual table =), it becomes just three lines of code in the
plugins instead of nine (setIsReady, setName and setIconName)
</pre>
</blockquote>
Hmm I agree on that... Definitely better than the current scheme.<br>
<blockquote cite="mid:200809130955.07604.aseigo@kde.org" type="cite">
<pre wrap="">
* the get methods are all slots but have return values. that's a bit odd =)
while you can certainly use slots in that way, generally slots are "call and
forget". what's the use case for having them as slots with return values in
this class?
</pre>
</blockquote>
Errr that doesn't make sense anymore (those functions used to be void,
a long time ago), I just forgot to change them to regular functions.<br>
<blockquote cite="mid:200809130955.07604.aseigo@kde.org" type="cite">
<pre wrap="">
* instead of getResources, etc ... we tend to drop the 'get' part in kde
libraries.. looking at other nepomuk code, similar methods use the 'find' prefix
(findPropertyBy*, findClassBy*). does it make sense to make that consistent
here?
</pre>
</blockquote>
considering the behaviour of the functions, findResources makes as much
sense as getResources do (maybe even more ;) ).<br>
<blockquote cite="mid:200809130955.07604.aseigo@kde.org" type="cite">
<pre wrap="">
* you could combine the three getResources into just two by cobining the first
two and providing null defaults for the last two parameters?
virtual AnnotationResult* getResources(const QString& filter, const
Types::Property& property = Types::Property(), const Soprano::Node& object =
Soprano::Node());
of course, that would require isEmpty or isNull checks available for
Types::Property and Soprano::Node ... i see that at least Entity has an
isValid method ...
* you have data members in the protected section, in particular
m_queuedCommands. that should be accessed by a getter and a dptr should be
added to the class.
</pre>
</blockquote>
OK too. Sebastian already made some remarks on the API, I just didn't
have time to take them into consideration yet (I went back to school 3
days ago).<br>
<blockquote cite="mid:200809130955.07604.aseigo@kde.org" type="cite">
<pre wrap="">
* the "for internal use only" enum and typdef ... would it make sense to add
that to the private class as well, to remove it from the public API?
so nothing big there either, at least based on what's already existing... what
were the plans for inclusion in nepomuk-kde of these classes? (timelines, etc)
if the goal is 4.2 (*cough* *wink* *cough* ;) that could work out rather well
for the file dialog.
p.s. does nepomuk-kde have a set coding style? the code in kdelibs isn't quite
the kdelibs style, and annotation plugin code is yet another style ..</pre>
</blockquote>
While the API cleanup will take a bit of time, it's not such a huge
amount of work either. I just have to block myself three hours to clean
that up...<br>
About the coding style : it should be the KDE coding style, except that
parts coded by me use tabs and not spaces (I won't troll about that,
and am OK to comply with spaces, I just have to run sed on my files).<br>
I'm a bit in a rush now, I have some school work and a TOEFL exam to
sort out :s I'll try to work a bit on AnnotationPlugins tonight though.<br>
<br>
Cheers<br>
Adrien<br>
<br>
</body>
</html>