Review Request: GSoC : Nepomuk plugin for Amarok

Matěj Laitl matej at laitl.cz
Mon Aug 20 22:46:08 UTC 2012 

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106042/#review17777
-----------------------------------------------------------


This is first round, I've checked just page 1, but the list is getting already long...


src/core-impl/collections/nepomukcollection/CMakeLists.txt
<http://git.reviewboard.kde.org/r/106042/#comment13978>

    1. This should be rather macro_optional_find_package( Nepomuk ) to allow users to disable building of it even when Nepomuk is present.
    
    2. Additionally, this should be followed bymacro_log_feature( NEPOMUK_FOUND "Nepomuk" ...
    see commented-out commands in top-level CMakeLists.txt.
    
    3. These commented-out things should be removed, too.
    
    4. Don't you also need soprano? Just guessing from the old commented-out code.



src/core-impl/collections/nepomukcollection/NepomukCollection.h
<http://git.reviewboard.kde.org/r/106042/#comment14004>

    There seems no thing that would benefit from using Meta:: namespace.



src/core-impl/collections/nepomukcollection/NepomukCollection.h
<http://git.reviewboard.kde.org/r/106042/#comment13987>

    Yes as soon as you allow meta-data changes of the tracks from within Amarok, because you'll have to emit updated() when any of your meta maps change. (see MapChanger usage by IpodCollection)



src/core-impl/collections/nepomukcollection/NepomukCollection.h
<http://git.reviewboard.kde.org/r/106042/#comment14003>

    Nicpick: you dont need to mention Collections::



src/core-impl/collections/nepomukcollection/NepomukCollection.h
<http://git.reviewboard.kde.org/r/106042/#comment13979>

    Bad wording, it seems that the instance is passed to the constructor.



src/core-impl/collections/nepomukcollection/NepomukCollection.h
<http://git.reviewboard.kde.org/r/106042/#comment13980>

    Constructor cannot return anything.



src/core-impl/collections/nepomukcollection/NepomukCollection.h
<http://git.reviewboard.kde.org/r/106042/#comment13981>

    Nitpick, correct formatting is:
    Collections::QueryMaker *queryMaker()
    
    (perhaps more occurences in this review)



src/core-impl/collections/nepomukcollection/NepomukCollection.h
<http://git.reviewboard.kde.org/r/106042/#comment13982>

    Even though 2-line, I would prefer it to be in .cpp.



src/core-impl/collections/nepomukcollection/NepomukCollection.h
<http://git.reviewboard.kde.org/r/106042/#comment13983>

    Nitpicky: no need to insert spaces between method declarations if they don't have doc strings.



src/core-impl/collections/nepomukcollection/NepomukCollection.h
<http://git.reviewboard.kde.org/r/106042/#comment13992>

    It should be documented that this method just fires ThreadWeaver job and returns quickly.



src/core-impl/collections/nepomukcollection/NepomukCollection.h
<http://git.reviewboard.kde.org/r/106042/#comment14000>

    Shoul be handled entirely by CollectionFactory - this shouldn't be even constructed if Nepomuk is not available.



src/core-impl/collections/nepomukcollection/NepomukCollection.h
<http://git.reviewboard.kde.org/r/106042/#comment13993>

    You don't seem to use this atribute anywhere, it should be removed in that case.



src/core-impl/collections/nepomukcollection/NepomukCollection.cpp
<http://git.reviewboard.kde.org/r/106042/#comment13986>

    Nitpick: I usually sort / character before - character in includes so that core is above core-impl.



src/core-impl/collections/nepomukcollection/NepomukCollection.cpp
<http://git.reviewboard.kde.org/r/106042/#comment13994>

    Move this to CollectionFactory so that you don't have dangling and unusable collection.



src/core-impl/collections/nepomukcollection/NepomukCollection.cpp
<http://git.reviewboard.kde.org/r/106042/#comment14001>

    Move to CollectionFactory



src/core-impl/collections/nepomukcollection/NepomukCollection.cpp
<http://git.reviewboard.kde.org/r/106042/#comment13988>

    No one can even catch this here, because no one can have pointer to you this time.



src/core-impl/collections/nepomukcollection/NepomukCollection.cpp
<http://git.reviewboard.kde.org/r/106042/#comment13989>

    Well, you added
    using Collections;
    so this shoudn't be needed right? Nitpick though.



src/core-impl/collections/nepomukcollection/NepomukCollection.cpp
<http://git.reviewboard.kde.org/r/106042/#comment13990>

    i18n!



src/core-impl/collections/nepomukcollection/NepomukCollection.cpp
<http://git.reviewboard.kde.org/r/106042/#comment13991>

    Note that Collection::isWritable() has nothing to do with ability to change track meta-data. It means that tracks can be copied to and removed from collection. Do you implement that?
    
    Also please remove commented-out code.



src/core-impl/collections/nepomukcollection/NepomukCollection.cpp
<http://git.reviewboard.kde.org/r/106042/#comment14013>

    Register the job with Amarok::Components::logger(), mentioning its abort() slot.



src/core-impl/collections/nepomukcollection/NepomukCollectionFactory.h
<http://git.reviewboard.kde.org/r/106042/#comment13995>

    You don't need this include



src/core-impl/collections/nepomukcollection/NepomukCollectionFactory.h
<http://git.reviewboard.kde.org/r/106042/#comment13996>

    You don't need this fwd-declare, you don't mention it in this file.



src/core-impl/collections/nepomukcollection/NepomukCollectionFactory.h
<http://git.reviewboard.kde.org/r/106042/#comment13997>

    My code style is to indent this to 4 spaces and methods to 8 spaces, but I understand that KDevelop (and perhaps other devs) think otherwise.
    
    But I think our code_style.txt has it indented with 4 & 8, not 0 & 4. Applies to other classes, fix it everywhere or leave it like this.



src/core-impl/collections/nepomukcollection/NepomukCollectionFactory.cpp
<http://git.reviewboard.kde.org/r/106042/#comment13998>

    m_initialised should be set in all cases and right on top.



src/core-impl/collections/nepomukcollection/NepomukCollectionFactory.cpp
<http://git.reviewboard.kde.org/r/106042/#comment13999>

    The code could be factored to reduce duplication:
    
    if( not initialised )
       if( not try to initialise )
          logger->longMessage(...) (move from NepomukCollection)
          return
    *collection = new NepomukColleciton(..)
    emit newCollection(collection)
    



src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.h
<http://git.reviewboard.kde.org/r/106042/#comment14006>

    No need to include, fwd-declaratio suffices



src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.h
<http://git.reviewboard.kde.org/r/106042/#comment14005>

    run() should be protected



src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.h
<http://git.reviewboard.kde.org/r/106042/#comment14007>

    code style: Collection *coll;



src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.h
<http://git.reviewboard.kde.org/r/106042/#comment14009>

    This is an important design issue:
    
    You either need MemoryCollection or all these hashes, but not both. I'm still not convinced that you cannot avoid MemoryCollection at all, but if you use it, don't duplicate MapChanger.



src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.cpp
<http://git.reviewboard.kde.org/r/106042/#comment14010>

    code style: declare variables right before use.



src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.cpp
<http://git.reviewboard.kde.org/r/106042/#comment14011>

    You don't need your hash just for this! Just do:
    
    m_memoryColl->artistMap().contains( artistLabel ) (or something similar, ensire that the map is (const)-referenced and to copied during the call) and you get the answer (and proper instance)! Or, is calling it.binding( "artist" ) that expensive??
    
    Same problems down from here.



src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.cpp
<http://git.reviewboard.kde.org/r/106042/#comment14012>

    You should emit totalSteps() to actual count of tracks the query found first for progress bar to be accurate.



src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.cpp
<http://git.reviewboard.kde.org/r/106042/#comment14008>

    collectionUpdated() is not a signal, but a hack.
    
    Better: make the Job have own updated() signal and:
    connect( job, SIGNAL(updated()), collection, SIGNAL(updated()) )


- Matěj Laitl


On Aug. 20, 2012, 11:17 a.m., Phalgun Guduthur wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106042/
> -----------------------------------------------------------
> 
> (Updated Aug. 20, 2012, 11:17 a.m.)
> 
> 
> Review request for Amarok, Edward Hades Toroshchin, Vishesh Handa, and Matěj Laitl.
> 
> 
> Description
> -------
> 
> Nepomuk plugin for Amarok.
> 
> Almost all of the code changes can be found in src/core-impl/collections/nepomukcollection/*
> And a minor change in src/core-impl/collections/support/MemoryMeta.cpp
> 
> Code builds and after Nepomuk plugin is activated in the "Settings" dialog, Nepomuk Plugin comes into play and queries all the tracks in your machine. The query is not 'that' fast and might take several seconds depending on the number of tracks in your box. 
> IMPORTANT : Make sure Nepomuk is enabled if you want to give the plugin a spin. 
> 
> 
> Diffs
> -----
> 
>   src/core-impl/collections/CMakeLists.txt c78b920 
>   src/core-impl/collections/nepomukcollection/CMakeLists.txt 7cfd4b0 
>   src/core-impl/collections/nepomukcollection/NepomukAlbum.h 185c25a 
>   src/core-impl/collections/nepomukcollection/NepomukAlbum.cpp 6a09a1b 
>   src/core-impl/collections/nepomukcollection/NepomukArtist.h 6fcedf3 
>   src/core-impl/collections/nepomukcollection/NepomukArtist.cpp 13ddf01 
>   src/core-impl/collections/nepomukcollection/NepomukCollection.h 928b145 
>   src/core-impl/collections/nepomukcollection/NepomukCollection.cpp cb185e8 
>   src/core-impl/collections/nepomukcollection/NepomukCollectionFactory.h PRE-CREATION 
>   src/core-impl/collections/nepomukcollection/NepomukCollectionFactory.cpp PRE-CREATION 
>   src/core-impl/collections/nepomukcollection/NepomukComposer.h 1b11325 
>   src/core-impl/collections/nepomukcollection/NepomukComposer.cpp f21251e 
>   src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.h PRE-CREATION 
>   src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.cpp PRE-CREATION 
>   src/core-impl/collections/nepomukcollection/NepomukGenre.h ce0e3b7 
>   src/core-impl/collections/nepomukcollection/NepomukGenre.cpp 945074c 
>   src/core-impl/collections/nepomukcollection/NepomukQueryMaker.h 50067de 
>   src/core-impl/collections/nepomukcollection/NepomukQueryMaker.cpp 33163ea 
>   src/core-impl/collections/nepomukcollection/NepomukRegistry.h a21347e 
>   src/core-impl/collections/nepomukcollection/NepomukRegistry.cpp 8afa199 
>   src/core-impl/collections/nepomukcollection/NepomukTrack.h 77dd8c7 
>   src/core-impl/collections/nepomukcollection/NepomukTrack.cpp 7db01cf 
>   src/core-impl/collections/nepomukcollection/NepomukYear.h 504cbe2 
>   src/core-impl/collections/nepomukcollection/NepomukYear.cpp 1f13de0 
>   src/core-impl/collections/nepomukcollection/amarok_collection-nepomukcollection.desktop 1ac9f02 
>   src/core-impl/collections/nepomukcollection/meta/NepomukAlbum.h PRE-CREATION 
>   src/core-impl/collections/nepomukcollection/meta/NepomukAlbum.cpp PRE-CREATION 
>   src/core-impl/collections/nepomukcollection/meta/NepomukArtist.h PRE-CREATION 
>   src/core-impl/collections/nepomukcollection/meta/NepomukArtist.cpp PRE-CREATION 
>   src/core-impl/collections/nepomukcollection/meta/NepomukComposer.h PRE-CREATION 
>   src/core-impl/collections/nepomukcollection/meta/NepomukComposer.cpp PRE-CREATION 
>   src/core-impl/collections/nepomukcollection/meta/NepomukGenre.h PRE-CREATION 
>   src/core-impl/collections/nepomukcollection/meta/NepomukGenre.cpp PRE-CREATION 
>   src/core-impl/collections/nepomukcollection/meta/NepomukLabel.h PRE-CREATION 
>   src/core-impl/collections/nepomukcollection/meta/NepomukLabel.cpp PRE-CREATION 
>   src/core-impl/collections/nepomukcollection/meta/NepomukTrack.h PRE-CREATION 
>   src/core-impl/collections/nepomukcollection/meta/NepomukTrack.cpp PRE-CREATION 
>   src/core-impl/collections/nepomukcollection/meta/NepomukYear.h PRE-CREATION 
>   src/core-impl/collections/nepomukcollection/meta/NepomukYear.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/106042/diff/
> 
> 
> Testing
> -------
> 
> Minimal. Plan to spend the remaining time on testing the plugin. 
> 
> 
> Thanks,
> 
> Phalgun Guduthur
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20120820/0a9ed440/attachment-0001.html>

More information about the Amarok-devel mailing list