Review Request: GSoC : Nepomuk plugin for Amarok

Matěj Laitl matej at laitl.cz
Fri Aug 24 09:20:22 UTC 2012 


> 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.
> 
> Edward Hades Toroshchin wrote:
>     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.
> 
> Matěj Laitl wrote:
>     > First, it would be much better to have implemented an own QueryMaker that would translate Amarok's queries to Nepomuk's queries.
>     
>     Total agreement.
>     
>     > 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).
>     
>     This is how MemoryCollection works. It is used for "dumb" back-ends that don't know tracks of an album etc.
>     
>     > However, it doesn't do this very reliably (or I didn't understand how to use it correctly),
>     
>     Well, it works as it is supposed to work. E.g. you should NOT find existing album (etc.) entities for new tracks you add to MemoryColleciton through MapChanger. You should return new instance every time (see IpodMeta::Track), MapChanger does the matching. Any matching you do yourself is completly unused.
>     
>     > so some quirks did creep out here and there. So, we switched to nepomuk-uid-uniqueness, handled by NepomukCollection.
>     
>     Has *no* effect at all, because the world sees the MemoryCollection maps, and it uses name-uniqueness. *No one* (apart from MapChanger for a short while) sees NepomukAlbum etc., and NepomukAlbum::tracks() is *never* called when using MapChanger. NepomukTrack is hidden behind MemoryMeta::Track. Test it!
>     
>     > 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.
>     
>     Understand that MemoryCollection is rather good temporary solution, but it is used incorrectly. (e.g. there's a lot of completely useless code)
> 
> Edward Hades Toroshchin wrote:
>     Again: we had used MemoryCollection as you describe and it didn't work well. I didn't want to dig into it (and I don't want to now).
>     
>     (if you don't take my word for it, or want to dig into it, please checkout some older commits from phalgun's branch).
>     
>     I understand how MemoryCollection works, and I understand that there is a certain effort duplication, as well as (some) wasted memory.
>     
>     So, to summarize, there are two options: either accept this uncool temporary solution, or switch it back to a cool temporary solution that works worse. I don't see much sense in the latter, because it's still temporary solution, and (if we believe phalgun) promises to be quite a short-lived one.
>
> 
> Matěj Laitl wrote:
>     > So, to summarize, there are two options: either accept this uncool temporary solution, or switch it back to a cool temporary solution that works worse.
>     
>     The "cool temporary solution" *cannot*, by definition, work worse. Read my above comment again, all that additional code has *no visible effect*. How can I convince you? (do you see that all these hashes are used only for short while when the NepomukConstructMetaJob runs and then they're discarded?)
>     
>     > I don't see much sense in the latter, because it's still temporary solution, and (if we believe phalgun) promises to be quite a short-lived one.
>     
>     The "cool temporary solution" would be a couple of hundreds code lines less, witch nearly no effort. Worth it IMO.
> 
> Edward Hades Toroshchin wrote:
>     You're talking theory, I'm talking practice. Been there, done it, gone away.
>     
>     I'm just saying we shouldn't waste our time perfecting a _temporary_ solution. And it will amount to some time wasted, because we already wasted it once, trying to use MemoryCollection as you propose. Of course, we could have done this wrong.
>     
>     If you still insist on reworking this, go ahead. I suggest you help phalgun on it then :)
> 
> Edward Hades Toroshchin wrote:
>     Also, you are abusing the term "by definition" :)
> 
> Bart Cerneels wrote:
>     Good thing there is good news w.r.t. nepomuk query speed: http://vhanda.in/blog/2012/08/faster-nepomuk-queries/
>     I suspect this was part of the need for MemoryQueryMaker use.
>     
>     I'm leaning to believe Matej w.r.t. the code duplication. This comes from decent understanding of MemoryCollection (wrote part of it) but without reading NepomukCollection code, at least not the latest version, fully.
>     
>     Pragmatically I would just merge this code, which is working and move to a full NepomukQueryMaker as soon as possible and if the performance is good enough. If that does not happen soon enough, code quality and maintainability (i.e. less complex code) is more important and regular use of MemoryQueryMaker should at least be attempted.
> 
> Phalgun Guduthur wrote:
>     Though I agree that using an extra hashmap might result in code redundancy, it serves the purpose temporarily. 
>     I would also prefer not restructuring it for now as I'm looking to write my own NepomukQueryMaker which executes queries on the fly instead of storing all the meta objects as it is being done now through MemoryCollection.

> Though I agree that using an extra hashmap might result in code redundancy, it serves the purpose temporarily.

Do you guys (apart from Bart) actually read me? It serves no purpose, but if you're already working on NepomukQueryMaker, leave it.

This gives me an idea - why about merging the code into master only when NepomukQueryMaker is ready and MemoryCollection ditched? Reviewing new and ready code is much easier (and we'll spot more things) than reviewing bad -> good code.


> 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.
> 
> Edward Hades Toroshchin wrote:
>     Soprano is a Nepomuk's dependency.
> 
> Phalgun Guduthur wrote:
>     Am I supposed to uncomment the macro_optional_find_package code in the top level CMakeLists.txt too? 
>

No, you're supposed to delete to commented-out code in top level CMakeLists.txt Just add your commands (incl. macro_optional_find_package) to src/core-impl/collections/nepomukcollection/CMakeLists.txt


> On Aug. 20, 2012, 10:46 p.m., Matěj Laitl wrote:
> > src/core-impl/collections/nepomukcollection/NepomukCollection.cpp, line 281
> > <http://git.reviewboard.kde.org/r/106042/diff/4/?file=79201#file79201line281>
> >
> >     Register the job with Amarok::Components::logger(), mentioning its abort() slot.
> 
> Phalgun Guduthur wrote:
>     I tried doing that, but Amarok::Components::logger() doesn't work with ThreadWeaver jobs directly right? 
>     Is there a way to use ThreadWeaver jobs with the logger? 
>
> 
> Bart Cerneels wrote:
>     Only KJobs (include KIO::Job) and cutom QObjects which emit the right signals. You can use the last one to make it work. If there are multiple threadweaver jobs that need to do the same (perhaps multiple inside your code) you should add support for it to Logger.

As Bart says, it works with any QObject, which your job is. Plus, you already have the signals ready in the job, just register the job with logger() and pass its abort slot to the registering call.


- Matěj


-----------------------------------------------------------
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/20120824/c8762d36/attachment-0001.html>

More information about the Amarok-devel mailing list