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

Laszlo Papp lpapp at kde.org
Fri Jul 8 05:52:00 CEST 2011 


> On July 7, 2011, 3:55 p.m., Arjen Hiemstra wrote:
> > 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...

Agreed. Actually apart from the setUiDelegate/uiDelegate, we can copy and paste the whole. This class seems to be quite generic to me for almost any job. It would actually even make sense to me (after the same thinking about the K{Archive,Zip,Tar,Ar} classes) to propose it upstream Qt with this new Open Governance model (Something like QAbstractJob/QJob ?). It is not even a long implementation, so we can just duplicate most of them without any issue. Also, we have then a ready made documentation as well, hehe. ;-)


> On July 7, 2011, 3:55 p.m., Arjen Hiemstra wrote:
> > player/lib/gamedetailsjob.h, line 77
> > <http://git.reviewboard.kde.org/r/101868/diff/1/?file=26176#file26176line77>
> >
> >     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...

Agreed, getter and storage make sense to me


> On July 7, 2011, 3:55 p.m., Arjen Hiemstra wrote:
> > player/lib/gamedetailsjob.cpp, line 179
> > <http://git.reviewboard.kde.org/r/101868/diff/1/?file=26177#file26177line179>
> >
> >     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.

Yeah, that is something I also advised previously in case of hard  coded numbers.


> On July 7, 2011, 3:55 p.m., Arjen Hiemstra wrote:
> > player/lib/ratingjob.h, lines 44-45
> > <http://git.reviewboard.kde.org/r/101868/diff/1/?file=26181#file26181line44>
> >
> >     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.

Yeah, I told my opinion like that to Shantanu previously because I also wanted this way. We also discussed that if we extend the API with new signals, it can be foobar{Finished/Failed/Running/Cancelled/etc} and in that sense it would make sense to use the naming schema like *{Finished/Failed/Running/Cancelled/etc}. This is something I am neutral about and that is why I was telling that leave it that way.

I think the more important bit is what I did in master. It is actually an uploading according to the method ( startRatingUpload ) and attica functiontionality. That renaming would more important for me, but it is done in main branch.


> On July 7, 2011, 3:55 p.m., Arjen Hiemstra wrote:
> > player/lib/serviceprovider.h, line 189
> > <http://git.reviewboard.kde.org/r/101868/diff/1/?file=26183#file26183line189>
> >
> >     failedToInitialize -> initializeFailed
> >     
> >     This signifies that the call to initialize, which triggered async behaviour, failed, instead of some magical process going on that failed.

Agreed, done in master.


> On July 7, 2011, 3:55 p.m., Arjen Hiemstra wrote:
> > player/lib/serviceprovider.h, line 193
> > <http://git.reviewboard.kde.org/r/101868/diff/1/?file=26183#file26183line193>
> >
> >     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.

Yeah, it is the same question like in case of the rating. I wanted to propose it this way, but after Shantanu wanted to keep it this naming schema, I became neutral about it.


> On July 7, 2011, 3:55 p.m., Arjen Hiemstra wrote:
> > player/lib/serviceprovider.h, line 197
> > <http://git.reviewboard.kde.org/r/101868/diff/1/?file=26183#file26183line197>
> >
> >     loggedIn -> loginFinished
> >     
> >     Again, method name + suffix == something happened in relation to the method.

Indeed, done in master.


> On July 7, 2011, 3:55 p.m., Arjen Hiemstra wrote:
> > player/lib/serviceprovider.h, lines 184-208
> > <http://git.reviewboard.kde.org/r/101868/diff/1/?file=26183#file26183line184>
> >
> >     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.)

For me, start and initialize are different terms. You can start anything, not just initialization. I agree about that what you mentioned below. It was also an issue for me seeing those since they imply slot names instead of pointint out, they are signals. *Starting is fine.


> On July 7, 2011, 3:55 p.m., Arjen Hiemstra wrote:
> > player/lib/serviceprovider.h, lines 203-208
> > <http://git.reviewboard.kde.org/r/101868/diff/1/?file=26183#file26183line203>
> >
> >     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 .

Yeap, agreed. That naming schema I was using in other codes as well. It was partially done in my previous commit into master.

After all, It seems there was a duplication and I already pushed a lot of changes apart from these comments into the master branch that made sense to me. I think we can close this review for now. When I finished the remaining part I wanted to do, I will drop Shantanu into the CCMAIL section and can fine-tune the further activities according to the shipped changes.

It should not take too long, hopefully during the daytime at some point today.


- Laszlo


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


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/20110708/c9353286/attachment-0001.htm

More information about the Gluon mailing list