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



File Attachment: ratings-main.png
<http://git.reviewboard.kde.org//r/112533/#fcomment95>
    instead of numbers, using stars would be better.
    
    the labels should also be center aligned:
    
    Usability: **
        Funny: ***
    


File Attachment: ratings-main.png
<http://git.reviewboard.kde.org//r/112533/#fcomment96>
    I wonder if we need an explicit "Ratings" header here. Replacing the raw numbers with stars (or some other icon, but stars are well recognized) should make it obvious that these are ratings. So it probably doesn't need to say that explicitly.
    
    A header here adds more visual noise and takes up screen space.


activeclient/package/contents/ui/settings/ParticipantRatings.qml
<http://git.reviewboard.kde.org/r/112533/#comment29036>

    this seems like a *very* literal presentation.
    
    imho it would be far, far nicer to group all the ratings for a given asset together and display it like it is in the main UI. this looks too much like a raw log output :)
    
    perhaps something like:
    
    <b>Poker 1 (v 1.1)</b>
        Usability: **
            Funny: ***
    
    
     



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

    this seems very brute force; just re-setting the assetId was not enough?
    
    i'm also a little concerned about popping the column from *inside* the column itself. i'm sure QML does the right thing behind the scenes w/regards to memory allocation, etc but it would be far nicer if simply re-setting the assetId would do the trick, or even if page offered a reload function that re-sent the asset info request.



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

    there was something about this button that just never felt right. it was Yet Another Button in an already pretty busy area of the UI.
    
    when looking at the Ratings page in the settings, it came to me: the Delete button should be there. each rating in the Participant Ratings list could have a delete button. that way a function which will be rarely used is not in the main application UI and is instead in the configuration / settings area where it fits in quite nicely.


- Aaron J. Seigo


On Sept. 5, 2013, 10:25 a.m., Giorgos Tsiapaliokas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112533/
> -----------------------------------------------------------
> 
> (Updated Sept. 5, 2013, 10:25 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
> 
> 
> Thanks,
> 
> Giorgos Tsiapaliokas
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/active/attachments/20130905/72a0544e/attachment-0001.html>