Review Request 112533: add ratings in the bodega-client

Mon Sep 16 08:56:57 UTC 2013

> On Sept. 14, 2013, 11:59 p.m., Aaron J. Seigo wrote:
> > lib/bodega/session.h, line 151
> > <http://git.reviewboard.kde.org/r/112533/diff/4/?file=189518#file189518line151>
> >
> >     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.

QHash<int, int> isn't consumable from QML, so I made it a QVariantMap. Is it ok now?

> On Sept. 14, 2013, 11:59 p.m., Aaron J. Seigo wrote:
> > activeclient/src/bodegastore.cpp, lines 325-326
> > <http://git.reviewboard.kde.org/r/112533/diff/4/?file=189499#file189499line325>
> >
> >     as noted in my previous review, this should be lazy initialized in participantRatingsJobModel()

so, with this change we don't need the private members anymore.
Right?

> On Sept. 14, 2013, 11:59 p.m., Aaron J. Seigo wrote:
> > activeclient/src/bodegastore.cpp, lines 327-328
> > <http://git.reviewboard.kde.org/r/112533/diff/4/?file=189499#file189499line327>
> >
> >     this should be lazy initialized in assetRatingsJobModel()

so, with this change we don't need the private members anymore.
Right?

> On Sept. 14, 2013, 11:59 p.m., Aaron J. Seigo wrote:
> > activeclient/package/contents/ui/storebrowser/AssetColumn.qml, line 282
> > <http://git.reviewboard.kde.org/r/112533/diff/4/?file=189495#file189495line282>
> >
> >     this is unecessary; the participant model is never visible at the same time as the asset browser UI. so this is just unnecessary overhead.

If I remove this line, and you add ratings into an asset and then you go
to the settings page, all the ratings that you added since the start
of the application won't be visible. No?

> On Sept. 14, 2013, 11:59 p.m., Aaron J. Seigo wrote:
> > activeclient/package/contents/ui/storebrowser/Ratings.js, lines 29-40
> > <http://git.reviewboard.kde.org/r/112533/diff/4/?file=189496#file189496line29>
> >
> >     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?

> why is this not using an associative array? it could then be as simple as: attribute[attributeId] = attributeValue.

I have ported the branch into the above.

> you can see the patch here: http://paste.kde.org/p7043787e/
> what do you think?

+1 for the idea, but I have one minor fix to this patch
the docs weren't updated :) http://paste.kde.org/p9b334632/

- Giorgos

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

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/20130916/95506605/attachment-0001.html>