Nepomuk Metadata Extractor moved to KDE Review

[Albert Astals Cid]

El Dimarts, 6 de novembre de 2012, a les 15:46:09, Jörg Ehrichs va escriure:
> Hi Albert,
> thanks for your input.
> I have a few questions about them though.
> 
> 
> 2012/11/5 Albert Astals Cid <aacid at kde.org>
> 
> > How are you handling the i18n in the lib part? It seems like you'd need a
> > KCatalogLoader in there.
> 
> i18n is something I never liked about KDE, it is a big magic box to me
> and i'm always glad if it works "somehow".

I guess you did not automagically learn how to code in C++, you read about it, 
asked people and then learnt. The same applies to i18n handling, it's just a 
big magic box because you never cared to learn

> is it enough to call
> K_CATALOG_LOADER(metadataextractor);
> 
> to make this work?

Basically yes.

> Or do I have to change anything else?
> 
> > Do we need to have a whole folder (sro) full of autogenerated files?
> 
> I had this in a README todo I totally forgot about it.
> I did add this locally, but this means a simple cmake run now takes
> around ~20min on a quite powerfull machine.

But you only need that on the first compile, no?

> I'm not sure if this is a really good solution. On the other hand, i
> don't have to deal with changes in the Shared Desktop Ontology
> and update these generated fiels always.
> 
> What do you think? should I upload the change and live with the very
> long compilation problem?

We don't keep moc files in the repo, keeping them would be faster compile 
times, but autogenerated files in a repo always seemed a bad idea to me.

> 
> > metadataparameters.h is installed but does not have the export nor uses a
> > d- pointer
> > publicationspipe.h is installed and missing a d-pointer, same for
> > nepomukpipe.h
> > There are also some classes with no private date but no d-pointer either,
> > if you need to expand
> 
> I've added d-pointer for all classes, except "metadataparameters.h".
> i'm not really sure how to transform this struct into a class with
> d-pointer.

You are also missing the export (if that class is suposed to be used 
externally, that i guess it is since you are installing it)

> Should I add getter/setter for each function? 

Yes

> Doesn't this make the
> dpointer stuff kinde pointless, as I need to add/change the functions
> everytime
> I add something (like I would do with the current way)

And? Adding functions is fine SC and BC wise, read 
http://techbase.kde.org/Policies/Binary_Compatibility_Issues_With_C++ on why 
d-pointers are important.

Cheers,
  Albert

> Or is there a way to "expose" the dpointer itself in a getter, so i
> can get all variables behind it?
> 
> Is there some other clever way to deal with it? The MetaDataParameter
> is merly a container that is used at different places,
> to hold all the information.
> 
> Kind Regards,
> Joerg