Review Request: Statistics and Achievements, part 2

Sat Jul 9 14:15:42 CEST 2011

> 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.
> 
> Felix Rohrbach wrote:
>     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.
> 
> Laszlo Papp wrote:
>     "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."
>     1) I have never said I do not have time for review, just that it was going to be postponed a bit. In fact, it was not even postponed.
>     2) The review is not about in one person in general, anybody can review it and I think splitted way would make sense in general after discussing with more KDE developers.
>     3) No way, it gets committed in this monolythic form into master. If it gets committed in splitted snippets, why not start asking for review as it is closer to the committing/merging way ?
>     
>     In general, if this patch is this way, I will make another reviews quite a few times. That is the only way how I can make sure I do not miss something. Do not forget I am just checking the patch format and some smaller programming mistakes. It needs also some review from the design point of view.
> 
> Felix Rohrbach wrote:
>     1) You didn't say how long you would need to postpone it. As you said before already I would need to wait one or two days, I thought the postpone would be a bit more than a few hours.
>     2) Well, of course more people can review it, but if a main reviewer prefers one method of reviewing, I'll take this.
>     3) Every method has its drawbacks. If I would just give you the commits I made one after another, 
>         a) you would asks for changes I maybe already did in a later commit
>         b) you would need to use my branch to see why I've done something, as I sometimes created classes in one commit and used them in another commit
>         c) I would need to rebase my branch to update the patches here which can easily lead to conflicts or broken commits with a 23 commits branch (not counting merged master commits)
>     
>     I think the separation in several subdirectories is the best way to go, as the patches are smaller then (but still not really small, esp. engine), but it doesn't have the drawbacks of the commit review method.
> 
> Laszlo Papp wrote:
>     Hi Felix,
>     
>     1) If I do not have time immediately, or in 1-2 days (as it is not my paid job), I think it still helps a lot for me if it is splitted when I can find the time for it.
>     2) There were more reviewers by default in question and I think it is generally a good idea to make it that way so that it causes as less issue for people as possible (ie.: the reasons mentioned below)
>     3) That is not a nice way of showing patch or merging back into the master in my opinion. That way the branch usage is not required. You could just work directly into the master. What I mean by "splitted" reviews is that what I wrote ("If it gets committed in splitted snippets, why not start asking for review as it is closer to the committing/merging way ?"). I would show the patches separately as they are candidates for pushing to master.
>     
>     "It's a bit long, but Arjen Hiemstra said this would be the best way." -> I tried to collect the reasons why I think personally it is not the best way:
>     
>     1) I have a very slow internet connection and every time I open this review request up, it takes me long time to load. 
>     2) It contains more logical units that are better to separate. For instance, I can take more serious technical reviews in my kdeext player and player lib part. 
>     3) I could not really take as fast quality review as possible meaning that you should wait for any feedback, if I would like to do it really that way (spending lot of time and just providing a full quality review)
>     3) I have never seen any GSoC/SoK students yet doing it this way, maybe for a valid reason. I know a few students who could make it nicely and I am sure you can also do.
>     4) I think it can only be better by splitting, at least I cannot mention anything it.
>     5) I had to do unneccesary overhead also on the mailing list configuration webpage because of the size of the patch in order get any review posted to the mailing list (I had to set up bigger size limit for the mailing list which is not nice in theory just because of this).
>     6) After the second, third, fourth review of the patch, I need to scroll over quite a few codes that I am not interested in anymore. It happens with a smaller patch as well, but in much smaller amount.
>     7) If you make patch based requests, you can have separate "REVIEW: xxx" entry in your separate patches which is not a bad idea in my opinion.
>     8) If you cannot split here, people might want to see the splitted paches before pushing to master anyway, for instance because of a windows testing (including myself).
>     
>     Just to make my purpose explicit. :) I would like to help you with reviewing from my leisure time, but without the unneccesary overhead I can think of and I experienced. I am not offensing you, but I think you do not take it that way. ;)
>

Ok, after some further discussion with Laszlo on IRC, I'll close this and provide smaller commits for review.

- 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/a40323f7/attachment.htm 