Review Request 108717: nepomuk: implement custom QueryMaker

Thu May 30 21:38:09 UTC 2013

> 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.
> 
> Edward Hades Toroshchin wrote:
>     It's not about consistency. Favouring the using-directive is not a question of style or preference.
> 
> Matěj Laitl wrote:
>     So it is a question of what? What is the (technical, now that you've excluded personal one) rationale for introducing this incosistency? (yes, I still view it as an inconsistency)
>     
>     I also don't like "issues" being marked are resolved here without ack of the one who opened them.
> 
> Edward Hades Toroshchin wrote:
>     About marked issues: reviewboard's workflow allows that, and we don't have any policy against it. My view is that this code will be maintained mostly by me, so any opened issues do not have a veto power over the patch, unless the issue creator specifically mentions, that the code with this issue unresolved is unacceptable.
>     
>     Regarding the using-directive specifically: the semantics of using-directive do not allow it to be a viable replacement for placing the definitions inside the namespace. Therefore consistency is not important.
>
> 
> Matěj Laitl wrote:
>     > and we don't have any policy against it.
>     
>     Well, I'd consider it just a good behaviour, which hopefully doesn't have to be adopted as another "policy". But I acknowledge that what is considered a "good behaviour" is culturally-dependent.
>     
>     > My view is that this code will be maintained mostly by me
>     
>     This assumption is always false. You are not working on your personal project. This is a community project, members of the community should listen to concerns raised by other members, else the community doesn't really work right.
>     
>     > Regarding the using-directive specifically: the semantics of using-directive do not allow it to be a viable replacement for placing the definitions inside the namespace.
>     
>     This is true, in general, but in all of "namespace Something {" occurrences in .cpp files in this patch there is no difference. Not a deal-breaker of course, but I'd appreciate more cooperative behaviour.
> 
> Edward Hades Toroshchin wrote:
>     > what is considered a "good behaviour" is culturally-dependent
>     
>     Did you mean that as an insult?
>     
>     > This is a community project, members of the community should listen to concerns raised by other members, else the community doesn't really work right.
>     
>     Which I do. I also believe members of the community should not put spokes into each others' wheels.
>     
>     > in all of "namespace Something {" occurrences in .cpp files in this patch there is no difference
>     
>     Which is obviously completely irrelevant.
>     
>     > I'd appreciate more cooperative behaviour
>     
>     How more cooperative could this get?

> Did you mean that as an insult?

Not at all, sorry it could have been interpreted as it. (still I'd like to encourage closing rb issues only after discussion)

> Which is obviously completely irrelevant.

I don't see why, but okay, I see you insist on keeping the code as-is, enough energy was spent discussing this very small detail.

> 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).
> 
> Edward Hades Toroshchin wrote:
>     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.
> 
> Matěj Laitl wrote:
>     Not for me. This isn't usual for other Amarok code and I wouldn't like to see such practice introduced. The current practice it to document pointer ownership properly in docstrings.
>     
>     I'd like to see this issue reopened. (ditto about closing issues without reviewer's ack)
> 
> Edward Hades Toroshchin wrote:
>     Self-documenting and self-policing interfaces are good, even if they aren't used often.
>     
>     (ditto about issues policy)
> 
> Matěj Laitl wrote:
>     > Self-documenting and self-policing interfaces are good, even if they aren't used often.
>     
>     I agree, but this could at least use QScopedPointer, which AFAICS is 1:1 equivalent to std::auto_ptr, modulo some method names. It has also more explicit behaviour (::take() instead of rhs-modifying assignment behaviour), which I actually consider more self-documenting that current solution.
>     
>     I still think memory management should be rather documented explicitly in this case.
> 
> Edward Hades Toroshchin wrote:
>     No it isn't equivalent. Quite unfortunately, because I don't like mixing std and Qt.
>     
>     However, if you still fail to understand the approach I've used here, I'll replace it with something simpler.

> However, if you still fail to understand the approach I've used here, I'll replace it with something simpler.

This is very likely, so please do. All the time I thought this just serves to say NepomukInquirer takes ownership of parser. Was it the case?

- Matěj

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108717/#review33230
-----------------------------------------------------------

On May 30, 2013, 7:12 p.m., Edward Hades Toroshchin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108717/
> -----------------------------------------------------------
> 
> (Updated May 30, 2013, 7:12 p.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/b13fa23a/attachment-0001.html>