> 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)
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. > 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. > > Edward Hades Toroshchin wrote: > See above. > > Matěj Laitl wrote: > To forther extend how MapChanger should be used: You should *not* find > any existing artist at all! Just pass textual artist name to NepomukTrack and > let it return some dumb Meta::ArtistPtr( new Artist( "Artist Name" ) ) in > each call to NepomukTrack::album(). Again, 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 > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
