Review Request 112533: add ratings in the bodega-client

[Aaron J. Seigo]

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


it is getting close :)


activeclient/package/contents/ui/storebrowser/AssetColumn.qml
<http://git.reviewboard.kde.org/r/112533/#comment29631>

    this is unecessary; the participant model is never visible at the same time as the asset browser UI. so this is just unnecessary overhead.



activeclient/package/contents/ui/storebrowser/Ratings.js
<http://git.reviewboard.kde.org/r/112533/#comment29632>

    why is this not using an associative array? it could then be as simple as: attribute[attributeId] = attributeValue.
    
    this could then just as well be a property in the ratingsBaloon item, making the whole thing a lot simpler.
    
    looking at the server side, we're passing around a *lot* of redundant strings in the json that imho aren't very useful.
    
    ratings: {
      { attribute: 1, rating: 2 }
      { attribute: 3, rating: 4 }
    }
    
    could just as easily be:
    
    ratings: { 1: 2, 3: 4 }
    
    i've got a patch here for the server which makes getting and setting ratings return that more compact model.
    
    this should help simplify the client-side code. 
    
    you can see the patch here: http://paste.kde.org/p7043787e/
    
    what do you think?



activeclient/src/bodegastore.cpp
<http://git.reviewboard.kde.org/r/112533/#comment29630>

    Are all of these job classed used in the QML?



activeclient/src/bodegastore.cpp
<http://git.reviewboard.kde.org/r/112533/#comment29628>

    as noted in my previous review, this should be lazy initialized in participantRatingsJobModel()



activeclient/src/bodegastore.cpp
<http://git.reviewboard.kde.org/r/112533/#comment29629>

    this should be lazy initialized in assetRatingsJobModel()



lib/bodega/session.h
<http://git.reviewboard.kde.org/r/112533/#comment29643>

    this exposes the bodega server API: the client needs to know what the ratings JSON should look like. 
    
    hiding the details of the server API is precisely why we have the Session class. it allows using bodega without being tied to a specific version of the server API. in future, if we adjust the server API, clients using libbodega won't need to be re-written as we can just change the implementation.
    
    instead of a QVariant, assetCreateRatings should take a QHash<int, int> (or similar) and convert that internally to the proper JSON. 



lib/bodega/session.cpp
<http://git.reviewboard.kde.org/r/112533/#comment29633>

    indentation


- Aaron J. Seigo


On Sept. 14, 2013, 10:35 a.m., Giorgos Tsiapaliokas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112533/
> -----------------------------------------------------------
> 
> (Updated Sept. 14, 2013, 10:35 a.m.)
> 
> 
> Review request for Bodega.
> 
> 
> Description
> -------
> 
> This patch implements the ratings feature
> 
> 
> Diffs
> -----
> 
>   activeclient/package/contents/ui/SettingsPage.qml fb46eca 
>   activeclient/package/contents/ui/settings/ParticipantRatings.qml PRE-CREATION 
>   activeclient/package/contents/ui/storebrowser/AssetColumn.qml a678f16 
>   activeclient/package/contents/ui/storebrowser/Ratings.js PRE-CREATION 
>   activeclient/package/contents/ui/storebrowser/RatingsColumn.qml PRE-CREATION 
>   activeclient/src/bodegastore.h 1e5aac5 
>   activeclient/src/bodegastore.cpp ba9dc27 
>   lib/bodega/CMakeLists.txt 8d382a7 
>   lib/bodega/assetjob.h 5aab88c 
>   lib/bodega/assetjob.cpp 5f539cb 
>   lib/bodega/assetoperations.h 7ce7900 
>   lib/bodega/assetoperations.cpp 9f9c2d5 
>   lib/bodega/assetratingsjob.h PRE-CREATION 
>   lib/bodega/assetratingsjob.cpp PRE-CREATION 
>   lib/bodega/assetratingsjobmodel.h PRE-CREATION 
>   lib/bodega/assetratingsjobmodel.cpp PRE-CREATION 
>   lib/bodega/globals.h 5ab45da 
>   lib/bodega/participantratingsjob.h PRE-CREATION 
>   lib/bodega/participantratingsjob.cpp PRE-CREATION 
>   lib/bodega/participantratingsjobmodel.h PRE-CREATION 
>   lib/bodega/participantratingsjobmodel.cpp PRE-CREATION 
>   lib/bodega/ratingattributesjob.h PRE-CREATION 
>   lib/bodega/ratingattributesjob.cpp PRE-CREATION 
>   lib/bodega/ratingsmodel_p.h PRE-CREATION 
>   lib/bodega/ratingsmodel_p.cpp PRE-CREATION 
>   lib/bodega/session.h d27d284 
>   lib/bodega/session.cpp a7c7e94 
>   lib/bodega/session_p.h ebefb4f 
> 
> Diff: http://git.reviewboard.kde.org/r/112533/diff/
> 
> 
> Testing
> -------
> 
> check the attached images
> 
> 
> File Attachments
> ----------------
> 
> 
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/09/05/ratings-main.png
> 
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/09/05/participantratings.png
> 
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/09/06/labelsnotcenter.png
> 
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/09/06/delete2.png
> 
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/09/06/brokenParticipant.diff
> 
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/09/06/participantbroken.png
> 
> 
> Thanks,
> 
> Giorgos Tsiapaliokas
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/active/attachments/20130914/49e37036/attachment-0001.html>