Review Request: GSoC : Nepomuk plugin for Amarok

Edward Hades Toroshchin edward.hades at gmail.com
Wed Aug 22 09:36:14 UTC 2012 


> On Aug. 20, 2012, 10:46 p.m., Matěj Laitl wrote:
> > src/core-impl/collections/nepomukcollection/CMakeLists.txt, line 1
> > <http://git.reviewboard.kde.org/r/106042/diff/4/?file=79195#file79195line1>
> >
> >     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.

Soprano is a Nepomuk's dependency.


> On Aug. 20, 2012, 10:46 p.m., Matěj Laitl wrote:
> > src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.h, line 71
> > <http://git.reviewboard.kde.org/r/106042/diff/4/?file=79206#file79206line71>
> >
> >     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.

There are two issues here.

First, it would be much better to have implemented an own QueryMaker that would translate Amarok's queries to Nepomuk's queries.

This would, of course, render MemoryCollection unneeded. However, we did not go that way in this project, and you can ask me (or phalgun) personally, if you want to learn the reasons.

Second, we tried to rely on MemoryCollection, and it didn't go very well. Main reason is that it mostly checks uniqueness by the uniqueness of titles (or title+artist for albums). However, it doesn't do this very reliably (or I didn't understand how to use it correctly), so some quirks did creep out here and there. So, we switched to nepomuk-uid-uniqueness, handled by NepomukCollection.

TL;DR: this is quasi-temporary solution as it is, as we will move to NepomukQueryMaker eventually. So let's leave these hashes where they are.


> On Aug. 20, 2012, 10:46 p.m., Matěj Laitl wrote:
> > src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.cpp, line 148
> > <http://git.reviewboard.kde.org/r/106042/diff/4/?file=79207#file79207line148>
> >
> >     code style: declare variables right before use.

This is a good guideline in general, but I don't think it'll look (or work) much cleaner (or better) in this particular case.


> On Aug. 20, 2012, 10:46 p.m., Matěj Laitl wrote:
> > src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.cpp, line 216
> > <http://git.reviewboard.kde.org/r/106042/diff/4/?file=79207#file79207line216>
> >
> >     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.

See above.


- Edward Hades


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


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/20120822/efe6d62d/attachment-0001.html>

More information about the Amarok-devel mailing list