Review Request: Add required classes, and extend existing classes to fulfill dependencies for AllGameItemsModel

[Arjen Hiemstra]

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


I would really recommend you create a base class for all these job classes. Right now, every one of them seems to have a different API. The Job API works very well for async behaviour, but I would recommend using a simplified version of http://api.kde.org/4.x-api/kdelibs-apidocs/kdecore/html/classKJob.html for a base class. Specifically, right now it seems the jobs do magic on creation, this seems rather error prone to me, especially if I somehow fail to directly connect to the job's data signal, considering that is apparently the only way of getting data from the jobs...


player/lib/gamedetailsjob.h
<http://git.reviewboard.kde.org/r/101868/#comment3825>

    What about storing the list of games and providing a simple getter? Right now I cannot really use a fire-and-forget method of working with this class...



player/lib/gamedetailsjob.h
<http://git.reviewboard.kde.org/r/101868/#comment3824>

    What is the difference between gameList and gameDetails? And why are they not separated into two individual classes?



player/lib/gamedetailsjob.cpp
<http://git.reviewboard.kde.org/r/101868/#comment3816>

    Hmm, you might want to put this in to some kind of external file. I expect these values will change at some point or another. They should probably be linked to the provider somehow.



player/lib/ratingjob.h
<http://git.reviewboard.kde.org/r/101868/#comment3823>

    Personally, I would remove the "rating" from these signals, and just make them finished/failed, since the class name already indicates the subject of the signal.



player/lib/serviceprovider.h
<http://git.reviewboard.kde.org/r/101868/#comment3820>

    While you're busy renaming stuff, please also rename a few of these, since they are not very consistent or imply other things than you want.
    
    In this case, please make it initialize() since that is the name you are also using for the signals. (And public API should avoid abbreviations wherever possible.)



player/lib/serviceprovider.h
<http://git.reviewboard.kde.org/r/101868/#comment3818>

    failedToInitialize -> initializeFailed
    
    This signifies that the call to initialize, which triggered async behaviour, failed, instead of some magical process going on that failed.



player/lib/serviceprovider.h
<http://git.reviewboard.kde.org/r/101868/#comment3819>

    providerInitialized -> initializeFinished
    
    As far as I can see, this is the only mention of "provider" in this class that is public API, so better to just hide it completely.
    
    Using this naming, it also signifies that the method "initialize" finished, similar to the failed signal.



player/lib/serviceprovider.h
<http://git.reviewboard.kde.org/r/101868/#comment3821>

    loggedIn -> loginFinished
    
    Again, method name + suffix == something happened in relation to the method.



player/lib/serviceprovider.h
<http://git.reviewboard.kde.org/r/101868/#comment3822>

    For these, the current names really imply they are slots, i.e. they imply that a task will be performed, instead of a task is currently being performed. I would suggest changing their naming to $actionStarting instead, for example "downloadStarting", since that implies the action is currently being performed. If you shuffle a bit with all the parts, you can even make all of them have a "subject-verb-action" order, something quite a bit closer to English sentence structure. For example, startUploadComments -> commentUploadStarting .


- Arjen


On July 6, 2011, 4:32 p.m., Shantanu Tushar Jha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101868/
> -----------------------------------------------------------
> 
> (Updated July 6, 2011, 4:32 p.m.)
> 
> 
> Review request for Gluon.
> 
> 
> Summary
> -------
> 
> This patch renames GameDetail to GameDetailsJob, Rating to RatingJob. It also extends both classes, adding more functionality. Also, renamed few signals for consistency. 
> 
> 
> Diffs
> -----
> 
>   player/lib/CMakeLists.txt f925fdf 
>   player/lib/gamedetail.h f179b47 
>   player/lib/gamedetail.cpp eb974b7 
>   player/lib/gamedetailsjob.h PRE-CREATION 
>   player/lib/gamedetailsjob.cpp PRE-CREATION 
>   player/lib/models/gameitemsmodel.cpp cecaad1 
>   player/lib/rating.h a614281 
>   player/lib/rating.cpp 941062d 
>   player/lib/ratingjob.h PRE-CREATION 
>   player/lib/ratingjob.cpp PRE-CREATION 
>   player/lib/serviceprovider.h 8844b7f 
>   player/lib/serviceprovider.cpp 21313eb 
> 
> Diff: http://git.reviewboard.kde.org/r/101868/diff
> 
> 
> Testing
> -------
> 
> Syntactically correct, compiles successfully. Actual testing to be done when AllGameItemsModel will be integrated.
> 
> 
> Thanks,
> 
> Shantanu Tushar
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/gluon/attachments/20110707/9bcc1bcd/attachment-0001.htm