Review Request: Statistics and Achievements, part 2

[Felix Rohrbach]

> On July 9, 2011, 6:06 a.m., Laszlo Papp wrote:
> > "It's a bit long, but Arjen Hiemstra said this would be the best way." -> Why it is not good idea is that I have a very slow internet connection and every time I open this up, takes me ages to load. Second, it really contains more things that should be separated. For instance, I can take more serious technical reviews in my kdeext player and player lib part. Right now I cannot see just them, I need to filter manually which is a bigger overhead than it could be. Furthermore, I could not really take a quality review on this patch since I did not have enough time and I still spent more 1-2 hours with it. It would really help to have separate review requests in order to provide fast and quality feedbacks for snippets.
> > 
> > I would really suggest in the future not providing god/monolythic diffs for review. For me personally, it is really not nice and a lot of overhead meaning I can easily lose my sake for any review.
> > 
> > Next step: please upload the player things in a separate review and I can go through them.

Sorry, but you told me you wouldn't have time, so I have done it the way Arjen Hiemstra wanted it to make it easier for him to review. I will create a second review only for the player subdirectory as you requested.


> On July 9, 2011, 6:06 a.m., Laszlo Papp wrote:
> > engine/abstractstatistic.h, lines 98-99
> > <http://git.reviewboard.kde.org/r/101887/diff/1/?file=26490#file26490line98>
> >
> >     This breaks the order public/protected/private order. Could you please move back if it is a public slot ?

hmm, I think you misunderstood the diff here a bit. I just moved some functions from protected to public, the order is still public/public Q_SLOTS/protected/private.


> On July 9, 2011, 6:06 a.m., Laszlo Papp wrote:
> > engine/achievementsmanager.h, line 88
> > <http://git.reviewboard.kde.org/r/101887/diff/1/?file=26493#file26493line88>
> >
> >     hasDependency as a common naming practice for such cases and also as the comment says ?

huh? hasDependency just checks if there is a dependency, while dependencySatisfied checks whether user has the dependency achievement.


- Felix


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


On July 8, 2011, 11:17 p.m., Felix Rohrbach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101887/
> -----------------------------------------------------------
> 
> (Updated July 8, 2011, 11:17 p.m.)
> 
> 
> Review request for Gluon.
> 
> 
> Summary
> -------
> 
> This is the second part of my SoK project, bringing statistics and achievements to gluon. While the first part included mainly the statistics and the databases to save them, this part includes the achievements and a way to show them in gluon player applications (atm there's only an implementation for the KDE Extended player), but also brings improvements to the statistics. It's a bit long, but Arjen Hiemstra said this would be the best way. And don't forget the second page of the diff ;)
> 
> 
> Diffs
> -----
> 
>   creator/plugins/docks/propertiesdock/CMakeLists.txt 05c5242 
>   creator/plugins/docks/propertiesdock/propertywidgetitems/longlongpropertywidgetitem.h PRE-CREATION 
>   creator/plugins/docks/propertiesdock/propertywidgetitems/longlongpropertywidgetitem.cpp PRE-CREATION 
>   creator/x-gluon-mimetypes.xml 378876d 
>   engine/CMakeLists.txt e8764f5 
>   engine/abstractstatistic.h 494032f 
>   engine/achievement.h PRE-CREATION 
>   engine/achievement.cpp PRE-CREATION 
>   engine/achievementsmanager.h PRE-CREATION 
>   engine/achievementsmanager.cpp PRE-CREATION 
>   engine/assets/CMakeLists.txt b8c087b 
>   engine/assets/other/achievements/CMakeLists.txt PRE-CREATION 
>   engine/assets/other/achievements/achievementsasset.h PRE-CREATION 
>   engine/assets/other/achievements/achievementsasset.cpp PRE-CREATION 
>   engine/assets/other/statistics/statisticsasset.h a1d1995 
>   engine/assets/other/statistics/statisticsasset.cpp a8d6372 
>   engine/booleanstatistic.h PRE-CREATION 
>   engine/booleanstatistic.cpp PRE-CREATION 
>   engine/game.cpp d5fc279 
>   engine/gameproject.h 805d193 
>   engine/gameproject.cpp 8d84024 
>   engine/gameprojectprivate.h 38c442a 
>   engine/gameprojectprivate.cpp 08424b1 
>   engine/gluon_engine_export.h 17fc00c 
>   engine/multiscorestatistic.h PRE-CREATION 
>   engine/multiscorestatistic.cpp PRE-CREATION 
>   engine/projectmetadata.h PRE-CREATION 
>   engine/projectmetadata.cpp PRE-CREATION 
>   engine/statistic.h e29b2fe 
>   engine/tasksstatistic.h PRE-CREATION 
>   engine/tasksstatistic.cpp PRE-CREATION 
>   player/examples/apocalypse.gluon/game.gluonmeta PRE-CREATION 
>   player/examples/ball.gluon/game.gluonmeta PRE-CREATION 
>   player/examples/invaders.gluon/game.gluonmeta PRE-CREATION 
>   player/examples/jumpnbump.gluon/game.gluonmeta PRE-CREATION 
>   player/examples/pong.gluon/game.gluonmeta PRE-CREATION 
>   player/kdeext/CMakeLists.txt 40da561 
>   player/kdeext/delegates/achievementdelegate.h PRE-CREATION 
>   player/kdeext/delegates/achievementdelegate.cpp PRE-CREATION 
>   player/kdeext/gamedetailsoverlay.h 99923a2 
>   player/kdeext/gamedetailsoverlay.cpp b6f525c 
>   player/kdeext/gamesoverlay.cpp d468186 
>   player/lib/CMakeLists.txt 5957748 
>   player/lib/models/achievementsmodel.h PRE-CREATION 
>   player/lib/models/achievementsmodel.cpp PRE-CREATION 
>   player/lib/models/commentsmodel.cpp 5a0bff0 
>   player/lib/models/gameitemsmodel.h c907f7b 
>   player/lib/models/gameitemsmodel.cpp 82f7202 
>   player/lib/models/gameviewitem.h 6a7f4ca 
>   player/lib/models/gameviewitem.cpp 67f0566 
> 
> Diff: http://git.reviewboard.kde.org/r/101887/diff
> 
> 
> Testing
> -------
> 
> It compiles and I tried to test every feature I implemented, but I didn't always tried every combination, so there might be still some bugs.
> 
> 
> Thanks,
> 
> Felix
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/gluon/attachments/20110709/0b5fe84d/attachment.htm