Review Request 108717: nepomuk: implement custom QueryMaker

Matěj Laitl matej at laitl.cz
Thu May 30 14:09:13 UTC 2013 

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


Very good work, Edward! I especially like how elegantly is the nepomuk query constructed and nicely separated classes that participate on that.

Sorry for being a bit of bitchy with code style, but please have in mind that by appliyng familiar coding style you're doing a favour to your reviewers, i.e. me. It is really much more pleasurable for me to review code written in a familiar code style than in $random_code_style_spitted_by_an_IDE.

My another concern is introducing non-necassary complexity, mainly d-pointers and some witty usages of C arrays. Please take into account that someone else might one day maintain the code, let's make it as easy as possible for her.

I think we can merge this even with a fair deal of TODOs, still there are some low-hanging fruits that I'd like to see fixed before merging.


src/core-impl/collections/nepomukcollection/NepomukCache.h
<http://git.reviewboard.kde.org/r/108717/#comment24582>

    I still don't favour introducing d-ptr pattern in new code, I see it as something needed just for binary compatibility which only makes code more complicated.
    
    Markey, what do you think?



src/core-impl/collections/nepomukcollection/NepomukCache.h
<http://git.reviewboard.kde.org/r/108717/#comment24584>

    not important nicpick, but this would be nice if this was (automatically?) converted to Amarok coding style, i.e.
    Type *pointer;
    const Something &reference
    
    I also don't add spaces around template arguments, but I acknowledge we're not really consistent about this:
    QSet<Type> set;
    
    I'm not going to repeat this for the rest of the patch.



src/core-impl/collections/nepomukcollection/NepomukCache.cpp
<http://git.reviewboard.kde.org/r/108717/#comment24583>

    Oh Edward, pretty please, don't stand on your way that hard.
    
    Ptr ptr = hash.value( key );
    if( !ptr )
    {
        // comment
        // comment
        ptr = new (...)
        hash.insert( ptr )
    }
    return ptr;
    
    one line less, one exit point less, one variable less. Pretty please. :-)



src/core-impl/collections/nepomukcollection/NepomukCollection.cpp
<http://git.reviewboard.kde.org/r/108717/#comment24585>

    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.



src/core-impl/collections/nepomukcollection/NepomukCollection.cpp
<http://git.reviewboard.kde.org/r/108717/#comment24586>

    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.



src/core-impl/collections/nepomukcollection/NepomukCollection.cpp
<http://git.reviewboard.kde.org/r/108717/#comment24635>

    Hmm, this would be really nice to have before merging, as the previous version implmeneted this, somehow.



src/core-impl/collections/nepomukcollection/NepomukCollection.cpp
<http://git.reviewboard.kde.org/r/108717/#comment24636>

    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.



src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.h
<http://git.reviewboard.kde.org/r/108717/#comment24637>

    what is the motivation for d-ptr pattern?



src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.h
<http://git.reviewboard.kde.org/r/108717/#comment24647>

    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).



src/core-impl/collections/nepomukcollection/NepomukParser.h
<http://git.reviewboard.kde.org/r/108717/#comment24651>

    Please add to documentation:
    
    This method is called from a non-main thread by NepomukInquirer, so be sure to take it into account (no GUI, no calling of non-thread-safe methods of non-local objects etc.)



src/core-impl/collections/nepomukcollection/NepomukParser.h
<http://git.reviewboard.kde.org/r/108717/#comment24652>

    Suggestion: m_seenUris



src/core-impl/collections/nepomukcollection/NepomukParser.cpp
<http://git.reviewboard.kde.org/r/108717/#comment24654>

    Either check the resulting pointer for validity or use ::staticCast(). The latter is fine, NepomukCache can be required to return just Nepomuk* entities.
    (more occurences below)



src/core-impl/collections/nepomukcollection/NepomukQueryMaker.cpp
<http://git.reviewboard.kde.org/r/108717/#comment24658>

    Hmm, this is the point where d-ptr pattern starts to confuse me a lot:
    Private class has its own methods and data members not prefixed with m_ (d->m_something would look weird, I agree)
    
    But in implementation of Private methods there is no distinction between local and class variable naming convention, which otherwise helps me a lot reading the code.



src/core-impl/collections/nepomukcollection/NepomukQueryMaker.cpp
<http://git.reviewboard.kde.org/r/108717/#comment24706>

    Suggestion: tiny comment that Q is actually "?" would be nice.



src/core-impl/collections/nepomukcollection/NepomukQueryMaker.cpp
<http://git.reviewboard.kde.org/r/108717/#comment24657>

    nitpick: one more indentation level to be consistent with other Amarok code.
    
    (I won't repeat this for occurences below)



src/core-impl/collections/nepomukcollection/NepomukQueryMaker.cpp
<http://git.reviewboard.kde.org/r/108717/#comment24659>

    This is not safe with Meta::val* values reorganization, also tricky to maintain. I have a suggestion:
    
    template<typename Key, typename Val>
    QHash<Key, Val> &
    operator<<( QHash<Key, Val> &lhs, const QPair<Key, Val> &rhs )
    {
        lhs.insert( rhs.first, rhs.second );
        return lhs;
    }
    
    NepomukQueryMakerPrivate::valueToSelector(qint64 bitValue)
    {
        static const QHash<qint64, QString> map = QHash<qint64, QString>()
            << qMakePair( Meta::valUrl, Q NS_trackUrl )
            << ...
    
    Not tested, so it may reqire tweaks (embodying Q NS_* into QString), but you get the idea.



src/core-impl/collections/nepomukcollection/NepomukQueryMaker.cpp
<http://git.reviewboard.kde.org/r/108717/#comment24660>

    TODO: use some actual function from Nepomuk API?  (doesn't need to be resolved, but at least the TODO should be extended to mention what is needed to be done)



src/core-impl/collections/nepomukcollection/NepomukQueryMaker.cpp
<http://git.reviewboard.kde.org/r/108717/#comment24707>

    wouldn't [ matchBegin * 2 + matchEnd * 1 ] look nicer? If that doesn't compile, perhaps
    [ int(matchBegin) * 2 + int(matchEnd) ]
    
    On the other hand, I'd still be happier if you used plain if/else, the performance penalty shouldn't be noticeable; or at least QStringList, or perhaps static QHash<QPair<bool, bool>, QString> using the trick above?
    
    QHash<QPair<bool, bool>, QString> = QHash<QPair<bool, bool>, QString>()
        << qMakePair( qMakePair( false, false ), "CONTAINS( str(%1), %2 )" )
        << ..
    
    should work, although it may look equally convoluted as the current solution. This is not a merge blocker, just a suggestion.



src/core-impl/collections/nepomukcollection/NepomukQueryMaker.cpp
<http://git.reviewboard.kde.org/r/108717/#comment24708>

    QScopedPointer please



src/core-impl/collections/nepomukcollection/NepomukQueryMaker.cpp
<http://git.reviewboard.kde.org/r/108717/#comment24709>

    Doesn't this fire more often than you want? i.e. if behaviour == TrackArtist && artist == 0?
    
    And perhaps the correct behaviour for AlbumArtists would be to fallback to TrackArtists until this is implemented? I haven't tested the effects though.



src/core-impl/collections/nepomukcollection/NepomukQueryMaker.cpp
<http://git.reviewboard.kde.org/r/108717/#comment24710>

    Doesn't this deserve at least TODO entries? Or this won't never be handled by NepomukCollection (in which case it needs some comment why)?



src/core-impl/collections/nepomukcollection/NepomukSelectors.h
<http://git.reviewboard.kde.org/r/108717/#comment24711>

    Nice, kudos for keeping this nicely separated and maintainable.



src/core-impl/collections/nepomukcollection/meta/NepomukAlbum.cpp
<http://git.reviewboard.kde.org/r/108717/#comment24712>

    This deserves at least a TODO now that you don't have MemoryCollection to do this for you. (applies to other meta classes except Track and Label, I won't repeat this)



src/core-impl/collections/nepomukcollection/meta/NepomukLabel.h
<http://git.reviewboard.kde.org/r/108717/#comment24713>

    Suggestion: make this on ordinary data member (non-pointer) or QScopedPointer<Nepomuk2::Tag> m_nepomukTag; (fwd declaration suffices in this case)



src/core-impl/collections/nepomukcollection/meta/NepomukTrack.h
<http://git.reviewboard.kde.org/r/108717/#comment24714>

    If we want to think about removing NepomukCollection at runtime, this should be QWeakPointer<NepomukCollection>, because you cannot control lifetime of the tracks (they may remain in the playlist ...).



src/core-impl/collections/nepomukcollection/meta/NepomukTrack.h
<http://git.reviewboard.kde.org/r/108717/#comment24715>

    I suggest using QScopedPointer<> for extra RAII kudo points.



src/core-impl/collections/nepomukcollection/meta/NepomukTrack.cpp
<http://git.reviewboard.kde.org/r/108717/#comment24716>

    minor: notifyObservers() called twice here. (doesn't really hurt)



src/core-impl/collections/nepomukcollection/meta/NepomukYear.h
<http://git.reviewboard.kde.org/r/108717/#comment24717>

    Suggestion: implement also virtual int year() const; to avoid excessive int -> QString -> int conversion.



src/statsyncing/collection/CollectionProvider.cpp
<http://git.reviewboard.kde.org/r/108717/#comment24718>

    Nice catch. We may think of putting this to Collection somehow, but this works fine for now.


- Matěj Laitl


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/813a69aa/attachment-0001.html>

More information about the Amarok-devel mailing list