Review Request: GSoC : Nepomuk plugin for Amarok

Matěj Laitl matej at laitl.cz
Sat Sep 15 12:11:27 UTC 2012 

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


This is a last round of nitpicks, I promise. ;) Otherwise ready. Before merging, add a commit with a line for ChangeLog.

Merge like this:
git checkout master
git merge --no-ff --log -e gsoc

Add REVIEW: <rr number>, DIGEST: Brief info, link to your blog, FEATURE: <look for bugs.kde.org for bugs resolved by your work> tags to the merge commit message. When you merge, don't immediately push to upstream, push only to your clone and pls. ping me to review the merge.


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

    Style: We usually define first public, then protected, then private.



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

    This variable is unused now.



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

    Nitpick:
    public -> public slots -> signals -> protected -> private.



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

    Marginally more cleaner way to do this is:
    trackPtr = TrackPtr( nepTrackPtr.data() );
    
    This prevents you to cast Track to NepomukTrack, but allows converting NepomukTrack to Track, something that ::staticCast() doesn't do.



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

    Unused fwd declare.



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

    Extra blank line.



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

    Redundant, delete.



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

    Just return m_artist; (which gets converted to bool)



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

    Unused fwd declare.



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

    Extra blank line.



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

    Unused fwd declare. (also a couple of times below)



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

    Extra blank line. (also a couple of times below)



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

    NEVAR EVAR use numeric identifers for enums. Use the symbolic ones! Plus our code style perhaps suggests one more indentation level for case ..:


- Matěj Laitl


On Sept. 15, 2012, 4:22 a.m., Phalgun Guduthur wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106042/
> -----------------------------------------------------------
> 
> (Updated Sept. 15, 2012, 4:22 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 c78b9202ece71b51189c4e47d85acfa4a74ef8d6 
>   src/core-impl/collections/nepomukcollection/CMakeLists.txt 7cfd4b056000cf5de18c87d1d014b6670703e796 
>   src/core-impl/collections/nepomukcollection/NepomukAlbum.h 185c25a0fe5b19248a3ab40c1d9d84fd66e6d2fe 
>   src/core-impl/collections/nepomukcollection/NepomukAlbum.cpp 6a09a1bbb4ea9bdfc08280326d29a351c666ab25 
>   src/core-impl/collections/nepomukcollection/NepomukArtist.h 6fcedf3ac3724083b6992deb71fb659d9b2dc5d0 
>   src/core-impl/collections/nepomukcollection/NepomukArtist.cpp 13ddf0142796d90af265d28a06d60110da64f138 
>   src/core-impl/collections/nepomukcollection/NepomukCollection.h 928b1458782f0145a012c81468f22edfafc0f547 
>   src/core-impl/collections/nepomukcollection/NepomukCollection.cpp cb185e818de2e00091f9cb03f4b19ccface14635 
>   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 1b11325ec488f202a7b13b10d36c8216b487ae89 
>   src/core-impl/collections/nepomukcollection/NepomukComposer.cpp f21251eab6798bb499d01900151b2c9a1783deae 
>   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 ce0e3b71515d88e57dd3d01beba85e3cdfd8ede6 
>   src/core-impl/collections/nepomukcollection/NepomukGenre.cpp 945074c4737ac2856469d5041ca2ea888d609bad 
>   src/core-impl/collections/nepomukcollection/NepomukQueryMaker.h 50067decec72f34a845e1da50e74cdf19e9c0f83 
>   src/core-impl/collections/nepomukcollection/NepomukQueryMaker.cpp 33163eaa0b279dedcf92de01346312930f10d944 
>   src/core-impl/collections/nepomukcollection/NepomukRegistry.h a21347eca2ab519a3c8b5b1f14650878fd7b4333 
>   src/core-impl/collections/nepomukcollection/NepomukRegistry.cpp 8afa199f73035eb7d95a8913eb1cbe9fea8b2ebd 
>   src/core-impl/collections/nepomukcollection/NepomukTrack.h 77dd8c70c8b0727655dfe1db89c7bd19208e77e5 
>   src/core-impl/collections/nepomukcollection/NepomukTrack.cpp 7db01cf34b3765f18f8b8b3cf6efbdf07af6e564 
>   src/core-impl/collections/nepomukcollection/NepomukYear.h 504cbe2b146ae9a53291de9e82fa384467eb14e1 
>   src/core-impl/collections/nepomukcollection/NepomukYear.cpp 1f13de0bd24e56b1b64b8c45f1d22720dd487a3c 
>   src/core-impl/collections/nepomukcollection/amarok_collection-nepomukcollection.desktop 815e69e492e819740aba620cc399a8ee79eace74 
>   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 
>   src/core-impl/collections/support/MemoryMeta.cpp 37ba510f61605af7ebd803aee3529bde18ad84c5 
> 
> Diff: http://git.reviewboard.kde.org/r/106042/diff/
> 
> 
> Testing
> -------
> 
> Minimal. Plan to spend the remaining time on testing the plugin. 
> 
> 
> Screenshots
> -----------
> 
> 
>   http://git.reviewboard.kde.org/r/106042/s/728/
> 
> 
> Thanks,
> 
> Phalgun Guduthur
> 
>

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

More information about the Amarok-devel mailing list