Review Request 108717: nepomuk: implement custom QueryMaker

Mon Feb 11 20:59:13 UTC 2013

> On Feb. 9, 2013, 5:14 p.m., Matěj Laitl wrote:
> > A lot of good work, Edward! Plus I really like that NepomukCollection takes the right direction with this patch.
> > 
> > There are some minor deficiencies and it seems that you've forgotten to apply Amarok coding style unfortunately.

Thanks for the review.

I decided not to add any docs yet, in case everything has to change.

I'll also fix the style issues after the non-style ones.

> On Feb. 9, 2013, 5:14 p.m., Matěj Laitl wrote:
> > src/core-impl/collections/nepomukcollection/NepomukCache.cpp, lines 52-60
> > <http://git.reviewboard.kde.org/r/108717/diff/5/?file=111844#file111844line52>
> >
> >     what about:
> >     Ptr ptr = hash.value( key );
> >     if( !ptr )
> >     {
> >         ptr = new (...)
> >         hash.insert( ptr )
> >     }
> >     return ptr;

Nah, I prefer it my way.

> On Feb. 9, 2013, 5:14 p.m., Matěj Laitl wrote:
> > src/core-impl/collections/nepomukcollection/NepomukCollection.cpp, lines 40-41
> > <http://git.reviewboard.kde.org/r/108717/diff/5/?file=111846#file111846line40>
> >
> >     "namespace Something {"
> >     is just ugly to contain the whole .cpp file. Why not preserve the good old "using namespace Collections;"?

I think using-directive is far uglier than a whole file in a namespace.

> On Feb. 9, 2013, 5:14 p.m., Matěj Laitl wrote:
> > src/core-impl/collections/nepomukcollection/NepomukCollection.cpp, lines 115-120
> > <http://git.reviewboard.kde.org/r/108717/diff/5/?file=111846#file111846line115>
> >
> >     Why removing this implementation? It looked well, but I dunno whether it isn't too expensive.

No, it's just wrong, and the possiblyContainsTrack()-trackForUrl() combo needs some special thinking, after the basics have settled.

> On Feb. 9, 2013, 5:14 p.m., Matěj Laitl wrote:
> > src/core-impl/collections/nepomukcollection/NepomukCollection.cpp, lines 123-138
> > <http://git.reviewboard.kde.org/r/108717/diff/5/?file=111846#file111846line123>
> >
> >     I see you'd need blocking QueryMaker here, right? Perhaps we could hack something generic out of StatisticsSynchronization.

> Perhaps we could hack something generic out of StatisticsSynchronization.

Told ya so.

Actually we probably won't need it here, but, again, let's think about it later.

> On Feb. 9, 2013, 5:14 p.m., Matěj Laitl wrote:
> > src/core-impl/collections/nepomukcollection/NepomukInquirer.h, line 30
> > <http://git.reviewboard.kde.org/r/108717/diff/5/?file=111849#file111849line30>
> >
> >     Again, at least basic documentation with purpose of this class would be nice.

Sure.

> On Feb. 9, 2013, 5:14 p.m., Matěj Laitl wrote:
> > src/core-impl/collections/nepomukcollection/NepomukInquirer.h, line 37
> > <http://git.reviewboard.kde.org/r/108717/diff/5/?file=111849#file111849line37>
> >
> >     At least the fact that it tooks over parentship of parser should be documented.

A'ight.

> On Feb. 9, 2013, 5:14 p.m., Matěj Laitl wrote:
> > src/core-impl/collections/nepomukcollection/NepomukParser.h, line 31
> > <http://git.reviewboard.kde.org/r/108717/diff/5/?file=111851#file111851line31>
> >
> >     docs!

You bet.

> On Feb. 9, 2013, 5:14 p.m., Matěj Laitl wrote:
> > src/core-impl/collections/nepomukcollection/NepomukParser.h, lines 53-68
> > <http://git.reviewboard.kde.org/r/108717/diff/5/?file=111851#file111851line53>
> >
> >     You could even make this
> >     template<typename MetaObjectList>
> >     bool parseOne( ..., MetaObjectList &objectList )
> >     
> >     with dummy generic implementation (that can jast assert(false)) and actual things implemented in template specializations. Would save just a couple of declarations though.

Yeah, I'll do that later. This code wasn't designed, it just evolved (not that I'm proud of it, of course).

> On Feb. 9, 2013, 5:14 p.m., Matěj Laitl wrote:
> > src/core-impl/collections/nepomukcollection/NepomukParser.cpp, line 48
> > <http://git.reviewboard.kde.org/r/108717/diff/5/?file=111852#file111852line48>
> >
> >     C-style casts are very dangerous in C++ code that uses virtual inheritance. (and you do because NepomukTrack inherits Meta::Statistics that inherits QSharedData virtually) (more occasions below, I won't repeat this)

Well it's okay in this case, but you're right of course. I just forgot we were using KDE's shared pointers, not Qt's.

> On Feb. 9, 2013, 5:14 p.m., Matěj Laitl wrote:
> > src/core-impl/collections/nepomukcollection/NepomukQueryMaker.h, lines 32-33
> > <http://git.reviewboard.kde.org/r/108717/diff/5/?file=111853#file111853line32>
> >
> >     Why the abortion of Amarok coding style?

It's easier for me to style code just before comitting.

> On Feb. 9, 2013, 5:14 p.m., Matěj Laitl wrote:
> > src/core-impl/collections/nepomukcollection/NepomukQueryMaker.cpp, line 175
> > <http://git.reviewboard.kde.org/r/108717/diff/5/?file=111854#file111854line175>
> >
> >     QHash or QMap would be much more nice, descriptive and safe, you can also construct it as static const using perhaps comma operator.

"Perhaps comma operator"? Haven't heard of it.

> On Feb. 9, 2013, 5:14 p.m., Matěj Laitl wrote:
> > src/core-impl/collections/nepomukcollection/NepomukQueryMaker.cpp, lines 205-214
> > <http://git.reviewboard.kde.org/r/108717/diff/5/?file=111854#file111854line205>
> >
> >     this ugliness could go away if you used QHash for map.

Nah, I'd just do the same with QHash::iterators, or just convert it to C array or something.

> On Feb. 9, 2013, 5:14 p.m., Matěj Laitl wrote:
> > src/core-impl/collections/nepomukcollection/NepomukQueryMaker.cpp, line 217
> > <http://git.reviewboard.kde.org/r/108717/diff/5/?file=111854#file111854line217>
> >
> >     better make this special value a static global string to prevent maintenance burden.

Yeah, there are a lot of consts for now, sorry about not telling you not to spot them :)

> On Feb. 9, 2013, 5:14 p.m., Matěj Laitl wrote:
> > src/core-impl/collections/nepomukcollection/NepomukQueryMaker.cpp, line 239
> > <http://git.reviewboard.kde.org/r/108717/diff/5/?file=111854#file111854line239>
> >
> >     ditto special value

ditto riposte

> On Feb. 9, 2013, 5:14 p.m., Matěj Laitl wrote:
> > src/core-impl/collections/nepomukcollection/NepomukQueryMaker.cpp, lines 265-271
> > <http://git.reviewboard.kde.org/r/108717/diff/5/?file=111854#file111854line265>
> >
> >     I wouldn't prefer such cryptic expressions to good old if/else. If you are concerned about overhead, you may still pre-create the static const QStrings.

Would it be ok if I add nice comments?

> On Feb. 9, 2013, 5:14 p.m., Matěj Laitl wrote:
> > src/core-impl/collections/nepomukcollection/meta/NepomukTrack.h, lines 175-176
> > <http://git.reviewboard.kde.org/r/108717/diff/5/?file=111864#file111864line175>
> >
> >     Looks like m_resource and m_resourceUri are mutually redundant, but null m_resource seems to be valid. What is the use-case for this? Perhaps you could just enforce m_resource isn't null and ditch m_resourceUrl?

Well, the tracks are created in-bulk from the results of SPARQL query (i.e. all the metadata is being retrieved), so there is no need to create a Nepomuk Resource object until it's really necessary.

> On Feb. 9, 2013, 5:14 p.m., Matěj Laitl wrote:
> > src/core-impl/collections/nepomukcollection/meta/NepomukTrack.h, line 48
> > <http://git.reviewboard.kde.org/r/108717/diff/5/?file=111864#file111864line48>
> >
> >     Is it allowed to construct a nepomuk track without collection set?

Sure.

- Edward Hades

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

On Feb. 3, 2013, 7:45 p.m., Edward Hades Toroshchin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108717/
> -----------------------------------------------------------
> 
> (Updated Feb. 3, 2013, 7:45 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 2c73fea67fd0f79d12d867907e055872372191aa 
>   src/core-impl/collections/nepomukcollection/NepomukCollection.cpp a85dc1ee1b7b987e501354d75364ed16d3f023cb 
>   src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.h 33167333a1a3537b30b1449232d4987bab3bd047 
>   src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.cpp 446346f3944e3b0ddf627d1d1b985006ffe27691 
>   src/core-impl/collections/nepomukcollection/NepomukInquirer.h PRE-CREATION 
>   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/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 27ff06d9e895d6d18b3f700d99ffaafb42288b4d 
>   src/core-impl/collections/nepomukcollection/meta/NepomukTrack.cpp 6d5bcf791ce88b42db9cf765b804ca5239d63c7d 
>   src/core-impl/collections/nepomukcollection/meta/NepomukYear.h 9eca44fbcd3aab77149d864cb6d3e5d266cc8461 
>   src/core-impl/collections/nepomukcollection/meta/NepomukYear.cpp 1d6e3b1066e13fd6adba15ae6b1d81f8ca28a5d9 
>   src/statsyncing/collection/CollectionProvider.cpp 84b7bf791c3b0f091549a0b643abada061360161 
> 
> 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/20130211/7db798f9/attachment-0001.html>