Review Request: GSoC : Nepomuk plugin for Amarok

Vishesh Handa me at vhanda.in
Mon Aug 20 09:23:29 UTC 2012 

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


This is just the first cursory glance. I'll look into it more later.

I really love review-board. We should have done this during the entire gsoc. :)


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

    You can check if Nepomuk is running via ResourceManager::initialized(), but you cannot start Nepomuk if it is not running, as the user would have explicitly disabled it from the system settings.
    
    For now, return true if Nepomuk is initialized, and display some kind of message if Nepomuk is not running, and then return false.
    
    The ResourceManager automatically initializes itself when it is first used, so you do not need to do that.



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

    Why are these member variables? Please make them to the NepomukContructMetaJob::run(). They don't need to exist outside that function.



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

    Nitpick- Move this line below the query.



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

    Minor nitpick - QString::fromLatin1( .. )



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

    ?r in the query refers to the MusicPiece, not to the album. So you'll never get any values for the album gain. It would be better to move this into the album block.
    
    Something like this -
    
    OPTIONAL {
         ?r nmm:musicAlbum ?albumRes .
         ?albumRes nie:title ?album .
    
         OPTIONAL {
             ?albumRes nmm:albumGain ?albumGain .
         }
    }



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

    Ditto.
    
    Always check the domain and range of all properties that you're using.
    
    http://oscaf.sourceforge.net/nmm.html#nmm:albumPeakGain



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

    nao:has?
    
    Please remove



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

    I'm slightly confused.
    
    Why does the track contain information about the album gain and album peak gain?



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

    This would still be the resource uri of the tag. Something in the form of nepomuk:/res/some-uuid.
    
    You'll need to fetch the tag label as well. You can do that in the query.
    
    select ?tagUri ?tag where { %1 nao:hasTag ?tagUri . ?tagUri nao:identifier ?tag . }


- Vishesh Handa


On Aug. 18, 2012, 5:58 p.m., Phalgun Guduthur wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106042/
> -----------------------------------------------------------
> 
> (Updated Aug. 18, 2012, 5:58 p.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/8db359c6/attachment-0001.html>

More information about the Amarok-devel mailing list