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/#review39876
-----------------------------------------------------------



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

    far, far too many jobs are being sent off to make this happen:
    
    * one job to request the participant ratings
    * another job per asset to fetch the ratings
    
    this makes no sense whatsoever.
    
    what this shows is that there is a problem with the server-side implementation of participant/ratings.
    
    it should be returning the data in the required end form itself already.
    
    i'll look into making that change so as not to cause another set of lengthy reviews there that blocks this even further.



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

    this should be created in participantRatingsJobModel, not here. it will rarely be accessed and only outside of the BodegaStore class, so delayed instantiation makes sense.



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

    m_participantRatingsJobModel should be instantiated here when requested:
    
    if (!m_participantRatingsJobModel) {
        m_participantRatingsJobModel = new ...
    }



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

    this method should:
    
    * perform a check for the existence of session; otherwise creating a ParticipantRatingsJobModel and then calling reload will cause a crash
    
    * check if the session is authenticated; if it is not, then it should connect to the session's authenticated(bool) signal and return. sth like:
    
    if (!session->isAuthenticated()) {
        connect(session, SIGNAL(authenticated(bool)),
                q, SLOT(fetchParticipantRatings()),
                Qt::UniqueConnection);
        return;
    }
    
    disconnect(session, SIGNAL(authenticated(bool)),
               q, SLOT(fetchParticipantRatings()));
    



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

    why would it automatically fetch the list on authentication? it should only fetch the ratings on authentication if ratings have been requested ... i think this can just go.



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

    comment out this noise before merging



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

    comment out this noise before merging


- Aaron J. Seigo


On Sept. 12, 2013, 11:29 a.m., Giorgos Tsiapaliokas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112533/
> -----------------------------------------------------------
> 
> (Updated Sept. 12, 2013, 11:29 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/20130912/921588f2/attachment-0001.html>