Review Request 108717: nepomuk: implement custom QueryMaker

Matěj Laitl matej at laitl.cz
Sat Feb 9 17:14:06 UTC 2013 

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


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.


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

    At least some basic usage documentation would be nice.



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

    Reminder for myself: remove d-ptr pattern once this is merged.



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

    At least basic documentation (e.g. whether these return null on invalid resource uri would be nice)
    
    Also Amarok coding style is: const QUrl &name (I won't repeat this for other occasions lower)



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

    Because the hashes here store KSharedPtrs, they prevent deletion of the once-created entities that are no longer referenced outside.
    
    As a memory-consumption optimization, you could store plain pointer and make NepomukCache a Meta::Observer and extend the new entityDestoryed() method to actually return the pointer.
    
    On the other hand, this should be done later and it is rather inappropriate for the first patch, so leave it like this for now.



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

    what about:
    Ptr ptr = hash.value( key );
    if( !ptr )
    {
        ptr = new (...)
        hash.insert( ptr )
    }
    return ptr;



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

    Our code style is that we put private data members on bottom, plus we prefer explicit private: label instead of the implicit one.
    
    Also the variable name could be tweaked, perhaps m_entityCache?



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

    "namespace Something {"
    is just ugly to contain the whole .cpp file. Why not preserve the good old "using namespace Collections;"?



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

    You actually change something that is already in Amarok coding style to something that isn't.



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

    Why removing this implementation? It looked well, but I dunno whether it isn't too expensive.



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

    I see you'd need blocking QueryMaker here, right? Perhaps we could hack something generic out of StatisticsSynchronization.



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

    Again, at least basic documentation with purpose of this class would be nice.



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

    Reminder to myself: remove d-ptr pattern once this is merged.



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

    At least the fact that it tooks over parentship of parser should be documented.



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

    docs!



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

    implicitly-private data member w/out the m_ prefix -> not nice.



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

    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.



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

    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)



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

    Why the abortion of Amarok coding style?



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

    QHash or QMap would be much more nice, descriptive and safe, you can also construct it as static const using perhaps comma operator.



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

    this ugliness could go away if you used QHash for map.



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

    



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

    better make this special value a static global string to prevent maintenance burden.



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

    ditto special value



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

    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.



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

    You should also add "|| behaviour == AlbumOrTrackArtists" until album artists are implemented.



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

    Matching against album entity should match all the fields that are in Meta::AlbumKey, currenlty: album name, album artist name. But perhaps this needs to wait until Nepomuk supports album artists. In either case at least TODO should be added.



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

    Amarok-style naming please (I won't repeat this for other entries)



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

    Would be nice if you added explicit keywords here, too.



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

    You memleak the nepomukTag, also Amarok-style naming...



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

    Is it allowed to construct a nepomuk track without collection set?



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

    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?



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

    You don't either need or should override this method, it isn't even virtual.



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

    You lack a couple of notifyObservers() here.


- Matěj Laitl


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/20130209/735d2ba3/attachment-0001.html>

More information about the Amarok-devel mailing list