----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106042/#review18939 -----------------------------------------------------------
Looks better now. I was a bit confused about my previous comments about usage of Nepomuk*Ptrs, the ones bellow are correct. Apart from this I consider it merge-ready provided you plan to work on this further. src/core-impl/collections/nepomukcollection/NepomukCollection.cpp <http://git.reviewboard.kde.org/r/106042/#comment15013> This has no effect as no one has pointer to Collection is its constructor. It is just confusing and should be removed. It is the trird time I'm saying this. src/core-impl/collections/nepomukcollection/NepomukCollectionFactory.cpp <http://git.reviewboard.kde.org/r/106042/#comment15014> nitpick: we don't usually add blank like between if and else - it helps to keep the related code together. Also we tend not to add blank like between undocumented methods in .h files (other places in the patch), but it is really minor. src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.cpp <http://git.reviewboard.kde.org/r/106042/#comment15015> What about my comments about moving this above the main query? src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.cpp <http://git.reviewboard.kde.org/r/106042/#comment15016> The explanation is rather strange. You said earlier that the ontologies are defined as doubles. If it is the case the right explanation would be that you need to use QString::toDouble() to be able to parse the real number stored as int. src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.cpp <http://git.reviewboard.kde.org/r/106042/#comment15018> Isn't the double -> int conversion automatic? I think so. If not, please use constuctor-style casts as C-style casts are something to avoid in C++ code [1]. [1] http://qt-project.org/wiki/Coding-Conventions section Casting src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.cpp <http://git.reviewboard.kde.org/r/106042/#comment15020> My reasoning about the KSharedPtrs: You dont have to typedef "your" (Nepomuk) KSharedPtrs at all. Just use Meta::*Ptrs everywhere (e.g. in NepomukTrack::setArtist() argument) and the code would work the same. The reason why I don't like typedefing new KSharedPtrs is that KSharedPtr<Derived> is *NOT* a sublass of KSharedPtr<Base>, which leads to ugly static_casts even in code that woudn't normally need them (passing Derived to code accepting Base). src/core-impl/collections/nepomukcollection/meta/NepomukTrack.h <http://git.reviewboard.kde.org/r/106042/#comment15021> Accept Meta::*Ptr and save a static_cast. (5x) - Matěj Laitl On Sept. 13, 2012, 6:58 p.m., Phalgun Guduthur wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106042/ > ----------------------------------------------------------- > > (Updated Sept. 13, 2012, 6:58 p.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/* > > 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. > > > Thanks, > > Phalgun Guduthur > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
