> 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. > > Matěj Laitl wrote: > > 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.
We wanted to maintain separate Nepomuk hashmaps to prevent duplicate Nepomukmeta objects. But I'm now realizing that my Nepomuk meta objects apart from NepomukTrack, serve no purpose and I could have just as used Meta::Artist, Meta::Album etc with the same functionality. If that is indeed true, then I could remove the HashMaps. I'll check this again in a few hours (don't have my laptop currently), and I'll change it according to what you said or give you an explanation as to why not. Thanks again! - Phalgun ----------------------------------------------------------- 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
