> On May 30, 2013, 2:09 p.m., Matěj Laitl wrote:
> > src/core-impl/collections/nepomukcollection/NepomukCollection.cpp, lines 40-41
> > <http://git.reviewboard.kde.org/r/108717/diff/11/?file=145267#file145267line40>
> >
> > Okay, personal preferences aside, I'm sure that consistency within a project has more value than preferences of individual developers.
> >
> > There are about 25 .cpp files in current Amarok source code that embody all the method definitions into namespace XYZ {}. On the other hand, there are about 265 files that use using namespace XYZ; syntax for function definitions. Please improve, rather than fight, consistency.
It's not about consistency. Favouring the using-directive is not a question of style or preference.
> On May 30, 2013, 2:09 p.m., Matěj Laitl wrote:
> > src/core-impl/collections/nepomukcollection/NepomukCollection.cpp, lines 93-102
> > <http://git.reviewboard.kde.org/r/108717/diff/11/?file=145267#file145267line93>
> >
> > I don't think you need to be an Observer anymore, do you? Or, it depends how you want to emit updated() signal, but as the tracks already have a pointer to NepomukCollection, they can somehow trigger it explicitly.
> >
> > I'm going to document where collections should emit updated() soon, it will read something like:
> > a) when a set of entities change (tracks, albums, composers,...) removed or added
> > b) when relationship between entities change, i.e. when tracks is assigned from one composer to another etc.
Detecting changes in Nepomuk is a complicated thing, which I don't want to mix with the currently discussed patch.
I suggest we discuss the Observer and update mechanisms together with it (and after we push this patch).
> On May 30, 2013, 2:09 p.m., Matěj Laitl wrote:
> > src/core-impl/collections/nepomukcollection/NepomukCollection.cpp, lines 118-120
> > <http://git.reviewboard.kde.org/r/108717/diff/11/?file=145267#file145267line118>
> >
> > We can use review request "issues" as a TODO list.
> >
> > We may have to decide whether the methods should accept the "file" urls or just the "Nepomuk" ones, but this is the general problem of trackForUrl(). We could split these into trackForFileUrl() and trackForUidUrl() in TrackProvider.
> >
> > Thinking about it more, we don't hopefully need the split and file:// urls should be accepted here too (if the file is tracked by Nepomuk), CollectionManager can enforce some order whey querying TrackProviders.
> We can use review request "issues" as a TODO list.
I don't like it, to be honest. How about creating a bug instead? For example "consistent usage of trackForUrl in collections"
> On May 30, 2013, 2:09 p.m., Matěj Laitl wrote:
> > src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.h, line 46
> > <http://git.reviewboard.kde.org/r/108717/diff/11/?file=145268#file145268line46>
> >
> > We prefer Qt equivalents of std classes, if they exist. But reading documentation of std::auto_ptr, it doesn't seem to make sense as a function argument, you doesn't seem to want to delete it as soon as it goes out of scope (and yeah, it may be reset by being on rhs of an assignment, but that is convoluted).
std::auto_ptr makes perfect sense as a function argument, because it both clearly shows who is responsible for the pointer deallocation and enforces it.
- Edward Hades
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108717/#review33230
-----------------------------------------------------------
On May 20, 2013, 10:57 a.m., Edward Hades Toroshchin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108717/
> -----------------------------------------------------------
>
> (Updated May 20, 2013, 10:57 a.m.)
>
>
> Review request for Amarok and Vishesh Handa.
>
>
> Description
> -------
>
> nepomuk: implement custom QueryMaker
>
>
> Diffs
> -----
>
> src/core-impl/collections/nepomukcollection/CMakeLists.txt 642919bd7b2188c6055308eabc07319ae48e14be
> src/core-impl/collections/nepomukcollection/NepomukCache.h PRE-CREATION
> src/core-impl/collections/nepomukcollection/NepomukCache.cpp PRE-CREATION
> src/core-impl/collections/nepomukcollection/NepomukCollection.h 5737cb5861563a8f190a29badc15d9e3ef82faf1
> src/core-impl/collections/nepomukcollection/NepomukCollection.cpp 8c35e3dda0e3dc9a23affe1d18cc69de4d24d58c
> src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.h 66e2473f16227f462c5a3838c71f311b6594333a
> src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.cpp 19743fe02b1d0c9dd4785d01909a4231b3a69e90
> src/core-impl/collections/nepomukcollection/NepomukInquirer.cpp PRE-CREATION
> src/core-impl/collections/nepomukcollection/NepomukParser.h PRE-CREATION
> src/core-impl/collections/nepomukcollection/NepomukParser.cpp PRE-CREATION
> src/core-impl/collections/nepomukcollection/NepomukQueryMaker.h PRE-CREATION
> src/core-impl/collections/nepomukcollection/NepomukQueryMaker.cpp PRE-CREATION
> src/core-impl/collections/nepomukcollection/NepomukSelectors.h PRE-CREATION
> src/core-impl/collections/nepomukcollection/meta/NepomukAlbum.h 8456ddb8d952e130816d01fc312c8df3146e83d1
> src/core-impl/collections/nepomukcollection/meta/NepomukAlbum.cpp 76c9136f9d4f5ba77cac49b242b3aa3b2d844c0c
> src/core-impl/collections/nepomukcollection/meta/NepomukArtist.h 7cf533e760b0a06e5a2316693e13f6aea0ac3dcd
> src/core-impl/collections/nepomukcollection/meta/NepomukArtist.cpp 34d7c8f9c32f96be0ef5f81de9d58bfedb510c2d
> src/core-impl/collections/nepomukcollection/meta/NepomukComposer.h 837c37fa5dce5c3c10e6840f618cf72430b51b3d
> src/core-impl/collections/nepomukcollection/meta/NepomukComposer.cpp 33ba85ae11100e8ccbb5cc94a2d7d0e2007fdbd1
> src/core-impl/collections/nepomukcollection/meta/NepomukGenre.cpp c34831ab4812f0241c0000eef844c2ea2397ae97
> src/core-impl/collections/nepomukcollection/meta/NepomukLabel.h 6f2e54c3c6a0d350615cea0e335ed9f5c5a0eedf
> src/core-impl/collections/nepomukcollection/meta/NepomukLabel.cpp 7e2a313e5eac91128d0ce211c6575ac3d8d2b1ab
> src/core-impl/collections/nepomukcollection/meta/NepomukTrack.h 9a534dddac51fc39f9d3705f95b194f9669416ab
> src/core-impl/collections/nepomukcollection/meta/NepomukTrack.cpp b9beca872cd7c7edc3e6b15662d0a49f42213c6d
> src/core-impl/collections/nepomukcollection/meta/NepomukYear.h 9eca44fbcd3aab77149d864cb6d3e5d266cc8461
> src/core-impl/collections/nepomukcollection/meta/NepomukYear.cpp 1d6e3b1066e13fd6adba15ae6b1d81f8ca28a5d9
> src/statsyncing/collection/CollectionProvider.cpp 7e3bdd83214796fc37e0aef41225baedabe3fbda
>
> Diff: http://git.reviewboard.kde.org/r/108717/diff/
>
>
> Testing
> -------
>
> Playing tracks from Nepomuk collection, browsing, filtering, adding/removing labels, statistics sync.
>
>
> File Attachments
> ----------------
>
>
> http://git.reviewboard.kde.org/media/uploaded/files/2013/02/03/nepomuk-counting-querymaker.png
>
> http://git.reviewboard.kde.org/media/uploaded/files/2013/02/03/nepomuk-queries.png
>
> http://git.reviewboard.kde.org/media/uploaded/files/2013/02/03/nepomuk-sync.png
>
>
> Thanks,
>
> Edward Hades Toroshchin
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20130530/27677809/attachment-0001.html>