Review Request 112533: add ratings in the bodega-client

Mon Sep 9 12:04:41 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/#fcomment99>
> >
> >     instead of numbers, using stars would be better.
> >     
> >     the labels should also be center aligned:
> >     
> >     Usability: **
> >         Funny: ***
> >
> 
> Giorgos Tsiapaliokas wrote:
>     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?

> I couldn't find a star icon.

the icon is called "rating" (e.g. actions/rating.png)

> my labels aren't aligned in the center.

as a first step, get them to line up on the ":"s so that the rating icons appear at the same x

category: ***
   other: ****

this will require having a label and then the images and right-aligning the labels to the same location, and the stars to the right of that.

> 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.
> 
> Giorgos Tsiapaliokas wrote:
>     I have moved the functionality of onAssetIdChanged to reloadPage and it works.

i probably was not clear enough: there should be no reason to re-create the asset column. currently it does this:

var page = itemBrowser.replace(Qt.resolvedUrl("AssetColumn.qml"))

it should simply be enough to reload the asset info without recreating the UI (which is expensive)

> 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.
> 
> Giorgos Tsiapaliokas wrote:
>     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?
>

The problem is precisely that each rating is a row in the model and that each row is showing up as its own row in the UI as a result.

There is *no reason* why a user should want to delete, for instance, just the rating given for "Usability" but leave the rating for "Performance" (or whatever) for the given asset. Ratings for an asset should be treated as one thing. That's how the server handles it for a reason: the UI makes the most sense when done this way as well.

In the user reviews listing, all of the ratings for a single asset should be shown together, *exactly* as it is in the asset info column.

- Aaron J.

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

On Sept. 6, 2013, 6:34 p.m., Giorgos Tsiapaliokas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112533/
> -----------------------------------------------------------
> 
> (Updated Sept. 6, 2013, 6:34 p.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/20130909/d8e3eb70/attachment.html>