Review Request 112533: add ratings in the bodega-client

Fri Sep 6 18:27:29 UTC 2013

> On Sept. 5, 2013, 10:47 a.m., Aaron J. Seigo wrote:
> > File Attachment: ratings-main.png
> > <http://git.reviewboard.kde.org/r/112533/#fcomment97>
> >
> >     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.

the Ratings header is gone

> On Sept. 5, 2013, 10:47 a.m., Aaron J. Seigo wrote:
> > File Attachment: ratings-main.png
> > <http://git.reviewboard.kde.org/r/112533/#fcomment98>
> >
> >     instead of numbers, using stars would be better.
> >     
> >     the labels should also be center aligned:
> >     
> >     Usability: **
> >         Funny: ***
> >

I couldn't find a star icon. Can you suggest one?
Also my labels aren't aligned in the center.

I have attached a new Image. What am I doing wrong?

> On Sept. 5, 2013, 10:47 a.m., Aaron J. Seigo wrote:
> > activeclient/package/contents/ui/storebrowser/AssetColumn.qml, lines 51-53
> > <http://git.reviewboard.kde.org/r/112533/diff/1/?file=187171#file187171line51>
> >
> >     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.

I have moved the functionality of onAssetIdChanged to reloadPage and it works.

> On Sept. 5, 2013, 10:47 a.m., Aaron J. Seigo wrote:
> > activeclient/package/contents/ui/storebrowser/AssetColumn.qml, lines 299-305
> > <http://git.reviewboard.kde.org/r/112533/diff/1/?file=187171#file187171line299>
> >
> >     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.

Each rating is a new row in our model. So if we have the asset 3 with two ratings
we will have two rows, so when I delete one of the ratings both of them will get deleted because
that's how our API works. It deletes all the ratings of an asset. I have also attached an image which
shows the new UI. Is that ok? Should we modify something else from the ui?

- Giorgos

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

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/20130906/406c3064/attachment-0001.html>