Hi Jörg,
On Tuesday, October 30, 2012 23:11:57 Jörg Ehrichs wrote:
> Please review the current codebase to help this getting as stable as
> possible.
I've actually gotten around to having a closer look at nepomuk-metdata-
extractor, built it, tried it and browsed the code for a while, which was a
pleasure. :) First of all, I must say that it doesn't work for me. It's a
local problem, my Kross misses Python support, I'm looking into that now.
I've collected a list of thoughts, questions and points for possible
improvements. Let me know what you think.
* The Architecture is I think very cool. Being able to extend it through
scripted plugins is a great that could make it hugely useful for a large
number of cases.
* As far as I can see, QWidget-related bits and "service-related" bits are
split, so we're able to drop another ui on top of it for Plasma Active without
much effort. (Even if the UI is a single knob =)) (Correct me if I missed a
few points.)
* The code overall looks clean, understandable and well structured. That's
good for maintainability.
* The browser integration is a very cool feature. It's nice that it supports
Konqueror, Chrome and Mozilla/NPAPI on multiple platforms. I suppose it works
with both Rekonq and Konqueror?
* Is there any support for images planned? I think especially additional
geolocation information could be really useful (also for other resource types,
by the way. In Plasma Active the user can set a location, it would be cool if
we could retrieve additional info through Nepomuk about the location, and
thereby being able to relate it to other data, think "Photos taken at current
location" =)
* One UI issue I see is that it installs a toplevel systemsettings module,
while it should be under "Desktop Search", so part of the Nepomuk KCM (which
is also not great at all, UI-wise :/). I could imagine the primary knobs
integration the service into the system a bit like this:
* Retrieve additional metadata from online sources [configure sources] on
first Nepomuk page
* Retrieve metadata for
[x] Music
[x] Video
[ ] Documents
[x] Websites
- limit simultaneous calls to sensible default (1-2-3?), remove ui option
- "check next" really needed?
- Do I need to enable the background service at all, "[ ] Fetch additional
metadata from online services", which enables or disables the service.
- The "non service way" is using
- it's unclear how it works if the background service is [ ]
- I don't know if the background service is actually doing something
(probably not, see above ;))
- maybe feedback could be done through the nepomuk notification
indicator?
If we want to add it to our default install by 4.11 (which I support!), those
should be fixed.
Note that you can still keep the options in the config, but putting all those
microknobs in which are really hard to understand especially on first setup
should be avoided. I think in most cases we can just choose sensible defaults,
for memory and performance critical stuff a bit more on the conservative side,
and get rid of most of the UI options, and in turn bring the actual content it
retrieves more into the picture. :)
* the binary name for metadataextractor should probably be nepomuk-
metadataexractor or somesuch (matching other related binaries, easier to find
if uses on the commandline).
- --force is more like --refresh, no biggie at all though
* Is the scripting API documented or is it planned?
* You're shipping pre-generated Nepomuk headers it seems, that seems to be bad
practice. Nepomuk has cmake "stuff" to generate the ontology headers for you,
those should be used. (In general, one of our policies is to not ship pre-
generated code, but all the tools to generate them, usually through the build
system. This is more in line with Free software practises, makes sure we
maintain everything that is needed to be able to regenerate the files. On top
of that, it makes interaction with Nepomuk more future proof, as you're
sharing the same ontologies, and more code. It also makes it harder to review,
since reviewing generated code is essentially useless.
You can use the nepomuk_add_ontologies cmake macro to generate those headers
for you (see kdelibs/cmake/modules/NepomukAddOntologyClasses.cmake for usage).
You can find an example of this in kde:plasma-crystal/CMakeLists.txt.
I think your solution to do a cmake-level switch is fine. The faster
generation is nice for quicker build cycles, but we have to include an easy
way to generate the headers.
* Coding Style
spacing after if statements:if(bla) becomes if (bla
spacing between arguments: foo(firstArg,secondArg) becomes foo(firstArg,
secondArg)
tabs in konqueror/metadataextractorplugin.cpp (maybe elsewhere, too)
* Licensing
Please consider using the KDE e.V. clause in your licenses, it makes the legal
side a bit more future proof (it's clearer about the (L)GPL successor). You
can find those on http://techbase.kde.org/Policies/Licensing_Policy From what
I can see, licensing overall pretty much complies otherwise, so that's just a
suggestion.
Cheers,
--
sebas
http://www.kde.org | http://vizZzion.org | GPG Key ID: 9119 0EF9