Nepomuk Metadata Extractor moved to KDE Review

Sebastian Kügler sebas at kde.org
Thu Nov 8 03:56:44 GMT 2012


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




More information about the kde-core-devel mailing list